-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix slow performance of polygon generation tool #30214
Merged
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
88fa6f6
Optimize `PolygonGenerationPopover`
minetoblend 9676989
Extract method for scheduler call
minetoblend 5c74fce
Remove objects when polygon is invalid
minetoblend 7e439be
Fix crash from non-serializable hitobject samples
minetoblend 8e781c1
Inline scheduler calls
minetoblend 9e97141
Fix off-by-one error
minetoblend f76a2d0
Make sure to properly update hitobjects
minetoblend 767ded4
Restore previous NewCombo state when adding objects
minetoblend 40339b8
Fix typo
minetoblend cd59b93
Merge branch 'master' into feat/optimize-polygon-tool
bdach 19f887e
Merge branch 'master' into feat/optimize-polygon-tool
peppy 1b279e4
Merge branch 'master' into feat/optimize-polygon-tool
bdach 88aea70
Do not permit new combo toggle to remain in indeterminate state on de…
bdach File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Make sure to properly update hitobjects
- Loading branch information
commit f76a2d02f61e522ae69eef5f1c3e8af0836bb42d
There are no files selected for viewing
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
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
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.
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.
I know that I proposed this originally but now I see that it's not correct in all instances. This change means that if you select an object that starts a combo, the new combo toggle will enable. Worse yet, if you select a combination of new-combo and non-new-combo objects, the toggle will enter indeterminate state and not even deselection will get rid of that state.
Not sure what the solution here is. Maybe the interaction of this popover with the new combo toggle should be removed completely. @peppy maybe you will have a UX opinion on this
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.
@bdach correct me if i'm wrong, but it seems the only change here is that the check for selected items has been added. so the fail case would be that "new combo" may not be reset when the selection is empty?
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.
Correct - in the worst case it will stay in indeterminate state after selecting and deselecting a group of objects with mixed new combo state.
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.
I think this is okay. The next toggle fixes the indeterminate state anyways (and in this state it will seemingly default to placement without new combo, which makes sense to me).
That said, @bdach if you're not happy with this interaction then I'd propose making the generate dialog not set new combo at all, and leave that up to the user to fix as a post operation.
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.
I've just made an alternative change to just revert the indeterminate state to full false on deselect (88aea70). Should hopefully be uncontroversial.
My only bother has ever been it visually looking like in the "indeterminate" state after a deselect, nothing more.