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

Improve group tracking logic to avoid switching which point type unnecessarily #18531

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 2, 2022

As pointed out by @smoogipoo , due to arbitrary parsing order, using the .First() type could cause the tracked type to switch on beatmaps which have two types of control points at the same time location.

Closes #18409

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally this also fixes the issue I pointed out in #18502 (comment)

Comment on lines +162 to +165
// If the selected group has more than one control point, choose the first as the tracking type
// if we don't already have a singular tracked type.
else if (trackedType == null)
trackedType = selectedGroup.Value?.ControlPoints.FirstOrDefault()?.GetType();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note - currently are only two possible control point types on this screen, and therefore, if there is more than one control point in the group, then all applicable types of control points are in the group.

If there are ever more, then this logic may lead to unexpected results. Imagine selecting a control point group with types [A, B], followed by selecting a group with types [C, D]. The first selection would cause the timing screen to start tracking A, but the second would not change tracking at all to either C or D (because there is more than one type in the group, and the tracked type is already set).

Not really worth fixing, just something that occurred in my mind in passing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's still best-effort. If we add more, I'd want the current selected "type" to show somewhere in the interface, and tracking be based on which you interacted with.

@peppy peppy mentioned this pull request Jun 3, 2022
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in voice, I don't agree with this tracking in the first place. So I'll just go with the flow for now.

@smoogipoo smoogipoo merged commit dd93fc2 into ppy:master Jun 3, 2022
@peppy peppy deleted the track-groups-better branch June 3, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor timing UX
3 participants