Skip to content
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

Limit Jar network to pairs only #29

Closed
wants to merge 5 commits into from

Conversation

DrParadox7
Copy link

@DrParadox7 DrParadox7 commented Jun 18, 2024

Refactored Mirrored Jars.
Jar Network capacity is now capped at 2, to trade only in pairs, significantly improving performance.
These jars will function exactly as expected as TC4 mirrors can only be paired.

It's important to note that this PR is a nerf, as Mirrored Jars Networks currently can hold infinite Mirrored Jars which will degrade performance massively relative to the network's size.

The existing mechanism continuously sorts an ArrayList of all jars to find those with the most and least essentia.
After transferring 1 Essentia, it restarts the entire process, causing substantial lag and being terribly inefficiency.

Fixes:
GTNewHorizons/GT-New-Horizons-Modpack#8724

Extra: Jar labels are no longer lost on placement.

Fixes:
GTNewHorizons/GT-New-Horizons-Modpack#4942 (It wasn't fixed)

@DrParadox7
Copy link
Author

DrParadox7 commented Jun 18, 2024

This gives a good overview:
https://spark.lucko.me/28gR5JOODu

That's a single player using them excessively in their setup

@Dream-Master Dream-Master requested a review from a team June 18, 2024 18:23
Refactor Mirrored Jars to only trade in pairs, massively enhancing performance.
Also fixes Jar labels being lost on placement
Pairing of jars always results into a new network, regardless of existing connections either jar had.

Networks in overcapacity will simply disconnect jars past the last valid connection rather than all its jars.
@Alastors
Copy link

I object to this PR, I get what you’re trying to do, but if performance is the issue, we need to look into the specifics of why, instead of massive nerfs that just crutch the issue.

@Glease
Copy link

Glease commented Sep 20, 2024

at first glance the thing tick the same network multiple time per tick (time equal to the number of connected jars). we can reduce cpu time to 1/10000 if one network is ticked only once per tick instead of once per tick per jar

EDIT: no it doesn't, but if sorting is the problem maybe try a sorted collection. I'd assume the network rarely updates so it doesn't need to be sorted once per tick

@Dream-Master Dream-Master added the ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version label Sep 20, 2024
@DrParadox7
Copy link
Author

I object to this PR, I get what you’re trying to do, but if performance is the issue, we need to look into the specifics of why, instead of massive nerfs that just crutch the issue.

Sorting the list keeps the level of all jars equal. This is a cause of a major TPS lag. My rework gets rid of it without losing its main functionality (which stays true to the original concept as mirrors work in pairs).

The fix already benefits its intended playerbase so whether the GTNH team adopts it or not is of no relevance to me.

I won't volunteer my time and lose myself in a process that does not benefit me but hope my PR inspires you to take matters on to your own hand.
As the saying goes:

"If you want something done right, do it yourself"

@Dream-Master Dream-Master removed the ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version label Dec 8, 2024
DrParadox7 and others added 3 commits December 15, 2024 07:54
Fixed wrong item reference in Damage Familiar Infusion
Clarify that Speed, Range and Damage familiar infusions are modifiers that require a valid effect infusion to be applied.

As it was worded, it implied these modifiers could be applied to the familiar's base attack, causing players to get unknowingly stuck on invalid infusions attempts.
@DrParadox7
Copy link
Author

@serenibyss You might want to include the label fix from the first commit as well to ensure nothing minor but worthwhile is overlooked from superseding this PR:

Also fixes Jar labels being lost on placement
https://github.com/DrParadox7/Gadomancy/blob/fb5bf61efd9ba0a55fe480aa2fee5e7a425ff6a8/src/main/java/makeo/gadomancy/common/items/ItemBlockRemoteJar.java#L133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants