-
Notifications
You must be signed in to change notification settings - Fork 2
Rewrite redstone interface and related mechanics to improve performance #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use per-world capability for manager - Change Redstone Activator to emit a signal based on the side clicked
- Fix deserialization for strong states - Prevent new signal removals being added to manager
- All signals with the same target are stored with their unique source - Signals are sorted in priority queue according to their strength
- Cache manager capability for tiles
- Pass block info to dynamic redstone - Properly notify neighbors immediately upon any change - Handle redstone remote message on main thread - Refactor signal removal to use subclass RemovalSignal - Change SignalQueue to be sorted based on total strength (weak + strong)
- Skip dynamic redstone checks if the power is already max
- Add toString() methods for signals
# Conflicts: # src/main/java/lumien/randomthings/handler/RTEventHandler.java
|
Wonderful PR! I won't be able to properly review it for a couple of days (includes the other PR as well), so I hope you can be patient! In the meantime, do you have any good benchmarks of the improvements in performance this provides? Also, feel free to edit the readme.md and changelog.md to add your changes and credit yourself, if you feel like it of course! |
|
Updated the docs with changes from this PR. Before PR: After PR: Permalink to reports: spark_profiles.zip The difference is probably more noticeable if you have many redstone interfaces/observers, in different dimensions. |
|
bugbot run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 2
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
src/main/java/lumien/randomthings/handler/redstone/DynamicRedstoneManager.java
Show resolved
Hide resolved
src/main/java/lumien/randomthings/handler/redstone/signal/RemovalSignal.java
Show resolved
Hide resolved
|
Everything looks good! Thanks for the PR! I'll mark 286, 493 and 517 as completed for now, and we can re-examine them if someone runs into any of those issues in the future. |
This PR rewrites or changes the following things in this mod:
All of these tiles/items now have their signals handled through a "dynamic" redstone manager; based on Integrated Dynamics' redstone writers' remote offset implementation (here). The manager is responsible for their remote redstone functionalities, and is implemented as a World capability.
Non-exhaustive list of notable changes:
World#getRedstonePower()/World#getStrongPower(), methods that are very frequently called by tile entities (See comments inAsmHandlerfor changes).RedstoneSignalHandlerasWorldSavedData).Related issues from original repo that are probably fixed: 286, 493, 517.