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 convert to stream functionality for juice streams #30763

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Darius-Wattimena
Copy link
Contributor

One of the more common tools used for catch mapping was converting a slider into a stream. This PR adds back this functionality, taking the osu editor convertToStream code as a basis.

There is some weird converting behaviour when you have a slider with many vertices, but that might be related to how sliders are created?

@bdach
Copy link
Collaborator

bdach commented Nov 19, 2024

There is some weird converting behaviour when you have a slider with many vertices, but that might be related to how sliders are created?

Explain please?

@@ -168,6 +190,48 @@ private void updateHitObjectFromPath()
lastSliderPathVersion = HitObject.Path.Version.Value;
}

private void convertToStream()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how to feel about this being a nigh carbon-copy of the osu! implementation. Especially so that I've heard osu! mappers call the current implementation of slider-to-stream "kneecapped" and wish for the stable one that could do more to be restored.

Probably would have preferred this was split to some common method in light of that, so that once that gets implemented, both rulesets reap the benefit. Or at the minimum link back both copies of the logic to each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to feel about this being a nigh carbon-copy of the osu! implementation. Especially so that I've heard osu! mappers call the current implementation of slider-to-stream "kneecapped" and wish for the stable one that could do more to be restored.

I wasn't aware that the current implementation was subpar; My intention for this PR was to have something in place instead of nothing at all. So might be better to have an improved version of this (and the osu version improved) in a follow-up PR.

Probably would have preferred this was split to some common method in light of that, so that once that gets implemented, both rulesets reap the benefit. Or at the minimum link back both copies of the logic to each other.

Good point. I'll rework it a bit so we have a shared implementation for the duplicate logic 👍
I initially didn't want to do this as I wasn't sure how overlapping the catch and osu logic would be, but it ended up almost the same...

@Darius-Wattimena
Copy link
Contributor Author

There is some weird converting behaviour when you have a slider with many vertices, but that might be related to how sliders are created?

Explain please?

Right now, creating a stream is based on the beat divider; however, if you have a slider that uses lots of vertices going left and right, then people might expect notes to be created that match how the vertices are shown instead of the location of the beat divider.

For example the following slider:

Which is now converted as this with a 1/4 beat divider:

Some people might expect different behaviours, such as having notes that match how the vertices are placed. However, this might be more of an enhancement than an actual issue.

@bdach
Copy link
Collaborator

bdach commented Nov 21, 2024

Right now, creating a stream is based on the beat divider; however, if you have a slider that uses lots of vertices going left and right, then people might expect notes to be created that match how the vertices are shown instead of the location of the beat divider.

Well that's intentional. The fruits have auditory feedback, as in catching them plays samples. So if things were allowed to convert at the nodes like you say, you'd essentially have unsnapped objects and mistimed samples playing for them. I don't see this as breakage.

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.

3 participants