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

[MC1.16] [Client] Performance improvements for grid view #2705

Merged
merged 6 commits into from
Jan 2, 2021

Conversation

ScoreUnder
Copy link
Contributor

See #2703. The changes are roughly the same as described there.

Testing this (and specifically, trying to reproduce the performance problems) was probably the hardest part. I stuck a bunch of importers around a chest and used a command block to fill it with junk:

loot insert 150 63 48 fish minecraft:gameplay/fishing/treasure 150 62 48

This got me a lot of non-stacking items, and it was just a matter of leaving this running for a while.

To elaborate on the bugs I mentioned in the previous PR:


Crafter manager search bug:

  1. Add some patterns to your RS system: grid, pattern grid, pattern, iron block
  2. Open your crafter manager
  3. Search for "pat gr"

Expected result: pattern grid shows up only

Actual result: pattern, grid, and pattern grid all show up as if you had typed pat|gr (which also does the same)


| at end of search bug:

  1. Have many items in RS
  2. Type @ref|

Expected result: Since the user has (probably) not finished typing, the @ref search should still be valid. Either that, or it should be equivalent to "@ref or <blank>", i.e. all items.

Actual result: Grid is blank


For completeness, screenshots (see perf graph on bottom left):

  1. No RS grid open
  2. RS grid open without sorting changes
  3. RS grid open with sorting changes

The regular lag spikes in 2 are from the importers constantly pulling in junk from the chest, and the grid view having to keep up with that. In 3 it looks like they're gone altogether (I did visually confirm that items were still jumping around in the grid). Note that frame times on the right-hand half of the graphs might be a little inflated because my screenshot utility is drawing above minecraft.

With this change, preventSortingWhileShiftIsDown can be safely disabled with negligible performance impact. Now it's just an option affecting gameplay, instead of performance overall, and can be enabled by anyone who doesn't like items jumping around under their cursor when they shift+click them.

@Darkere
Copy link
Collaborator

Darkere commented Oct 10, 2020

Ah you misunderstood what I meant
The grid screen has a specific function for NOT preventing sorting when shift crafting. Removing that would have prevented sorting when shift crafting and potentially improved performance.

Your solution is much better ofc 😄

@ScoreUnder
Copy link
Contributor Author

ScoreUnder commented Oct 10, 2020

Just noticed while playing around that this fixes another problem... The search #: is pretty slow (for obvious reasons I guess) when Advanced Tooltips (f3+h) is enabled, so before this change if anything is actively importing/exporting in a large inventory, depending on how large the inventory is and how fast the imports/exports are happening, it has the potential to completely freeze the client as it can't keep up with changes from the server. With this change, after the initial lag spike, that search is mostly fine.

@raoulvdberge
Copy link
Collaborator

This looks cool! I will try to review this PR this weekend.

@ScoreUnder
Copy link
Contributor Author

Rebased to fix merge conflicts

Plus refactor getFilters to avoid passing raw lists of filters around
and instead use GridFilter objects that combine conditions through OR
and AND.
This also follows the Law of Demeter a litle better.
Instead, use binary search and insert.
@ScoreUnder
Copy link
Contributor Author

Rebased again, same reason

@raoulvdberge raoulvdberge merged commit 74ab430 into refinedmods:mc1.16 Jan 2, 2021
@raoulvdberge
Copy link
Collaborator

Thanks!

@ScoreUnder ScoreUnder deleted the mc1.16-perf-updates branch January 5, 2021 21:42
ScoreUnder added a commit to ScoreUnder/refinedstorage that referenced this pull request Jan 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants