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

Enhance beatmap editing flow by adiding a audio tab #31291

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

Conversation

normalid-awa
Copy link
Contributor

Why

In my mapping experience I'm suffering from setting volume, banks, samples by using the tiny little popover, while I have to repeat it on every single hit object. Therefore, I took the inspiration from the fl studio's piano roll where user can by clicking set the samples, banks and volumes or other possible stuffs.

Classes

  • HitSoundTrackSamplePointBlueprint and NodeHitSoundTrackSamplePointBlueprintcontain the settings logic and the toggles/control drawable generation. HitSoundTrackSamplePointBlueprint corresponds to one hit object, while NodeHitSoundTrackSamplePointBlueprint corresponds to the node sample of IHasRepeats.
  • HitSoundTrackSamplePointBlueprintContainer attached to the timeline and contain the HitSoundTrackSamplePointBlueprint and NodeHitSoundTrackSamplePointBlueprint generation/update logic.
  • HitSoundTrackSamplePointToggle require HitSoundTrackSamplePointBlueprint and NodeHitSoundTrackSamplePointBlueprint, and call the Toggle method on the blueprint to manipulate the samples
  • HitSoundTrackSamplePointVolumeControl a vertical component and send the OnVolumeChange event on the blueprint to manipulate the samples
  • HitSoundTrackSamplePointVolumeControlBlueprint inherent from HitSoundTrackSamplePointBlueprint but overrides some method to support volume editing
  • NodeHitSoundTrackSamplePointVolumeControlBlueprint same as above

Showcase

osu.2024.12.27.-.00.09.34.01.mp4

Add support of repeated hit object
Extract colour logic to display component
@bdach
Copy link
Collaborator

bdach commented Dec 26, 2024

1.6k line diff with a completely new screen with zero prior discussion on the topic, especially with something like #31266 already open and several discussions about doing this sort of thing in the past?

I'm tempted to just close this on the spot and tell you to do this the proper way which is to ask first and then to split this correctly.

@normalid-awa
Copy link
Contributor Author

normalid-awa commented Dec 27, 2024

1.6k line diff with a completely new screen with zero prior discussion on the topic, especially with something like #31266 already open and several discussions about doing this sort of thing in the past?

I'm tempted to just close this on the spot and tell you to do this the proper way which is to ask first and then to split this correctly.

This pr is different from #31266 , #31266 is mainly focused on timeline editing while this pr is focused on hit sound editing, And about the splitting of pr, I've thought about splitting the screen and the children component with several pr, but I think it's meaningless since they are mostly used in one place so I guess they're not much requirements to sap them apart.

in addition, I think this pr offers a solution for those advice:
#31263

2. hitsound volume input should be automatically selected when clicking the pink badge below objects. 99% of the time i clicked these badges, it was to change volume, so having to click the volume input box each time sucks.
6. samplesets should be selectable. stable does this already, and having to type them in manually in lazer is nuts (#30786)
9. there should be a way to handle variable volume for slider bodies + spinners. it's common to change volume for a spinner or sliderticks gradually depending on a song's buildup/fadeout. for spinners, it's especially common to have different volume for the impact at the end vs the spinning portion (partially related to #29338)

@bdach
Copy link
Collaborator

bdach commented Dec 27, 2024

This pr is different from #31266 , #31266 is mainly focused on timeline editing while this pr is focused on hit sound editing

https://discord.com/channels/188630481301012481/188630652340404224/1275349781799243817 then. That looks better than what you have here quite frankly. Because at least it contains a preview of gameplay.

And about the splitting of pr, I've thought about splitting the screen and the children component with several pr, but I think it's meaningless since they are mostly used in one place so I guess they're not much requirements to sap them apart.

It's not meaningless to me. Frankly I do not trust that you can write good code yet, nor that you are able to make UX decisions that would more or less match ours. So I want to examine this code thoroughly and if it's in a 1.5k line blob then there are high chances this is going to end up in review purgatory.

I refuse to review this until it's split into more digestible parts. I'm personally not even sure I want this screen as it's shown over that discord message I linked at the start.

@peppy
Copy link
Member

peppy commented Dec 27, 2024

Ignoring code quality and size of PR, the current UI/UX of this really breaks my head. The "toggles" don't visually look like toggles and there's nothing showing the hitobjects themselves in line with the settings which are being adjusted.

I think anything which is extending hitobject settings should be in line with the timeline view to give a 1:1 correlation to what is being operated on.

ie. the timeline would expand down to expose the hitsample areas, like hinted in the designs:

2024-12-27 22 01 24@2x

@rust2
Copy link

rust2 commented Dec 29, 2024

I was thinking of doing something similar as a standalone application for myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants