Cleanup deprecated metadata access for tools #233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
S-S-X
added
Cleanup
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 tasks
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
S-S-X
force-pushed
the
issue-114-tools
branch
2 times, most recently
from
6f4f378
to
4673c37
Compare
This comment has been minimized.
This comment has been minimized.
S-S-X
commented
Nov 4, 2021
S-S-X
force-pushed
the
issue-114-tools
branch
from
4673c37
to
d9cb150
Compare
S-S-X
commented
Nov 4, 2021
This was referenced Nov 4, 2021
OgelGames
reviewed
Nov 5, 2021
OgelGames
reviewed
Nov 5, 2021
OgelGames
reviewed
Nov 5, 2021
S-S-X
force-pushed
the
issue-114-tools
branch
from
d9cb150
to
20b5ef8
Compare
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.04% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
|
Cleaned up commits. Previous: To: Maybe commit "5f28b16 Use ephemeral sounds for tools" is not needed, only kept it because that one is not exactly about power tool API but more generic stuff that just happens to be here... Some latest stats for LoC against current master (ec3a52c):
|
Sorry for the delay, test-server is up and running with restore from
No errors on startup, haven't tested anything else yet. |
I'll try to test this one later today so no need to worry about it. Will also try to advertise it a bit for some chat discussion if there's other people who'd be interested in some testing 🥼 |
|
Everything seemed to work, did not test all tools on pandorabox test server yet as I've been testing also on my own dev server. edit. SwissalpS told that "existing in-world bat-packs work fine, only newly placed ones are nerfed :)" |
|
@OgelGames problem with power bank is that it is trying to use Technic power tool API on nodes: Not really sure if that is exact problem or if it is just mistake using node name when item name should be used. |
Yep, I think that is the problem, I might have to change how the placing of powerbanks works... 🤔 |
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.03% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
S-S-X
commented
Mar 19, 2022
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.03% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.03% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
3 tasks
|
Will merge this soon to get clean table for future things, waiting for some time but this has been tested since... I don't know anymore.. but probably gone through more testing than any PR before merge so far and with a many testers too. I'll add some quick fix for power banks through fork + additional pr first if not updated by then. After that will be updating other mods if needed (and perma-forking those if that's what it takes). |
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.07% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
Cleanup deprecated metadata access for tools Fix mining drill translations Remove deprecated power tool functions Drop charge value completely Ensure use_RE_charge always uses at least 1 point Use precalculated wear factor, remove negative set_wear hack... Use technic_max_charge instead of max_charge in item definition, accept both Log errors instead of failing with invalid power tool stack Metadata migration for prospector Cleanup tool definition max_charge, get_RE_charge log warning for unknown stacks
Digtron battery holder compatibility
Add some tests for charging and using tools Fix tool spec, mod loading was updated Tests using air as default node
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.33% in 11/14 files:Test coverage report for technic chests 45.56% in 6/6 files:Test coverage report for technic 62.04% in 96/96 files:Raw test runner output for geeks:CNC: Chests: Technic: |
BuckarooBanzay
approved these changes
Jun 6, 2022
|
Powerbanks compatibility done here: https://github.com/S-S-X/powerbanks/tree/technic-plus and this fork should be used, this time I also did quick testing in game and it seems to work fine. Did not open PR for powerbanks compatibility. |
|
I've pushed a commit to OgelGames/powerbanks#8 to fix the bug (sorry for forgetting about that). I tested it with this PR, the current master and the I'll be merging the powerbanks PR later today, so if there's anything you think I've missed, let me know :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cleanup metadata access functions used by Technic power tools.
See issue #114
Option 1, requires compatibility shimsVerify that new metadata access is fully compatible with old tools.Old partially charged tools should work after update.Charge level should not change during update.Verify that tools energy usage has not changed.Tools continue playing with manual wear and charge storage reads/writes, storage cannot be updated easily in future.Requires dirty workarounds if deprecated functions will be removed (still manual serialization and data storage in meta field with empty key).
Option 2, break compatibility (31abc4e)
For this PR option 2 requires at least allowing charge value input/output for
technic.use_RE_charge.API proposal, tools never touch wear or charge values directly:
technic.use_RE_charge(itemstack, amount)technic.get_RE_charge(itemstack)technic.set_RE_charge(itemstack, charge)Testing
Crashes which is expected as I did remove all guards and wanted it to cause error if someone calls function with invalid stack. However checks are simple and crashes are not fun:accepts invalid stacks and logs red error to notify server admin about problems in some mod code.This list is going towards checking actual digtron bugs, methods are copy paste from digtron masterusable mineunit tests included but disabled (chainsaw mk3 but it is basically same)
This tool will lose configurationData migration added for configuration
✔️ some automated tests included
✔️ some automated tests included
on_refill, charging/discharging rejected)✔️ some automated tests included
✔️ some automated tests included
Notes
262 insertions(+), 419 deletions(-)excluding compatibility shims, reduces lines of code significantly while providing usable API.Compatibility
technic_addons.zip
Minimal compatibility Emojigit/technic_grass_clean#3
Full compatibility: minetest-mods/mychisel#9
Minimal compatibility berengma/antipest#1
Minimal compatibility berengma/farming_nextgen#19
(linked just one file / repo, some repos contain more)