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

Add slider-to-stream conversion operation to osu! editor #15583

Merged
merged 4 commits into from Nov 12, 2021

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Nov 11, 2021

2021-11-11.23-17-00.mp4

Addresses (closes?) #14246.

  • Accessible via hotkey the same way as in stable (Ctrl+Shift+F), or via the context menu on selection.
  • No dialog like on stable (yet?) - always uses the current snap divisor and uses the entire duration of the slider.
  • Supports sliders with repeats, contrary to stable, which seems to give up after the first span (unless I'm checking wrong?)
  • Preserves new combo, samples (taken from slider head) and sample control points in the conversion process.

All of the above has corresponding test coverage.

I know a stream placement tool was proposed, but I think for proper save/load support that may need to wait for a new beatmap format, otherwise there's no real good way to tell what was a stream after saving to legacy. If waiting for new format is seen as preferable I'll close this.

public override MenuItem[] ContextMenuItems => new MenuItem[]
{
new OsuMenuItem("Add control point", MenuItemType.Standard, () => addControlPoint(rightClickPosition)),
new OsuMenuItem("Convert to stream", MenuItemType.Destructive, convertToStream),
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Not sure about this being "destructive" (makes sense from a terminology perspective, but is also very red).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, was in two minds about that too. Could see what people say about it, or I could just fix in post.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Leave for now, if I felt strongly enough I would have already changed it. Maybe "highlighted" or some other in-between accent state works better here, not sure.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree with "Highlight" better than "Destructive".

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Keep in mind "highlight" is supposed to only be used on one option, where that option is the "normal" path forward. So it probably shouldn't be the answer here.

@peppy peppy merged commit a754b40 into ppy:master Nov 12, 2021
@bdach bdach deleted the convert-to-stream branch November 12, 2021 05:25
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.

None yet

3 participants