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

Move Ignored properties into IBeatmap #23418

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

Conversation

Ryooshuu
Copy link

@Ryooshuu Ryooshuu commented May 6, 2023

Closes #20883

@bdach bdach added area:beatmap-parsing .osu file format parsing realm deals with local realm database labels May 6, 2023
@bdach
Copy link
Collaborator

bdach commented May 6, 2023

Comment on lines 56 to 58
int[] Bookmarks { get; set; }

CountdownType Countdown { get; set; }
Copy link
Collaborator

@bdach bdach May 6, 2023

Choose a reason for hiding this comment

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

So while I don't disagree with doing this in the first place (since I'm the one who suggested it), I don't think this goes far enough. At the least the same thing should also be done to CountdownOffset:

/// <summary>
/// The number of beats to move the countdown backwards (compared to its default location).
/// </summary>
public int CountdownOffset { get; set; }

because it's weird that now half of the countdown info is in IBeatmap and half of it is in BeatmapInfo. But really, the same logic can be extended to everything in this #region.

If there is a time to move that stuff out of there, now is probably one of the best times to do so. @ppy/team-client would appreciate opinions on this matter.

If we do decide to move everything then it may be better to wrap all of that in some object rather than move everything to IBeatmap, because ProgressiveCalculationBeatmap is going to grow by quite a large margin if moving properties 1:1. But maybe that's fine.

Copy link
Author

@Ryooshuu Ryooshuu May 6, 2023

Choose a reason for hiding this comment

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

I was afraid to remove it since I didn't want to cause any unnecessary Realm changes. Rethinking it now, I don't think it would cause many issues. I'll wait for a response from others, however.

I'm a bit curious however about how I would name the new class if we decide to go in this direction, I'm currently thinking of BeatmapDetails but I'm unsure if that'll stick.

Copy link
Member

Choose a reason for hiding this comment

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

#23432 strangely relevant here.

I agree with moving all of these out as a first step.

If we do decide to move everything then it may be better to wrap all of that in some object rather than move everything to IBeatmap, because ProgressiveCalculationBeatmap is going to grow by quite a large margin if moving properties 1:1. But maybe that's fine.

I think if anything it should be a separate interface, rather than a separate class.

Copy link

Choose a reason for hiding this comment

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

I think if anything it should be a separate interface, rather than a separate class.

I decided to make a proper interface and a class called IBeatmapSettings so that it can be easily added to IBeatmap, and it condenses all of the options into one proper area without inflating the size of classes such as ProgressiveCalculationBeatmap with properties.

osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 8, 2023
@furiner
Copy link

furiner commented May 8, 2023

it appears as if master got forcefully improperly merged

@pull-request-size pull-request-size bot added size/M and removed size/L labels May 8, 2023
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 8, 2023
@@ -265,11 +265,11 @@ private void handleGeneral(string line)
break;

case @"Countdown":
beatmap.Countdown = Enum.Parse<CountdownType>(pair.Value);
beatmap.Settings.Countdown = Enum.Parse<CountdownType>(pair.Value);
Copy link
Member

Choose a reason for hiding this comment

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

what makes this a setting? as per my comment, i don't think this should be in a separate class.

maybe editorsettings could exist, but this is not that.

Copy link

@furiner furiner May 8, 2023

Choose a reason for hiding this comment

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

I believe Settings is the best term for these properties because they are all values that can be assigned and set by a medium such as the editor to a beatmap. The other only good term would've been Metadata, but that's already taken.

Furthermore, I also believe that taking the other properties from BeatmapInfo would be a good move as they are all in the same field.

Copy link
Member

@peppy peppy May 8, 2023

Choose a reason for hiding this comment

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

Disagree.

Now there's IBeatmap.Settings, BeatmapInfo.UserSettings and then everything in between (ControlPoints are also "settings" but they aren't in settings --- where do you draw the line? What about BaseVelocity? A setting or a variable or something else? WHo knows.).

Strongly against adding a new usage of the raw term "Settings" anywhere liek this.

The only usage of settings I would be okay with is to move editor level settings into its own storage, separate from variables and properties which are used at gameplay runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that "setting" isn't good, and agree in principle with splitting an interface for this additional stuff. The biggest issue I have though is how to name that new interface (classic...)

Like... best I can think of is probably IGameplayBeatmapProperties or something? And that still sucks.

Copy link

Choose a reason for hiding this comment

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

IBeatmapProperties is far better if we're going to keep the word "Properties".

@bdach
Copy link
Collaborator

bdach commented May 8, 2023

Also what is happening with the history here, out of curiosity? There are two github users committing to one branch? How does that work?

@furiner
Copy link

furiner commented May 8, 2023

There's nothing really stopping two people from collaborating on the same branch, and the pull request will reflect every commit any party creates to it; which is the entire gist to how maintainers can even push commits to branches on forks that aren't theirs.

Here, we're doing it because we're collaboratively working on the same exact issue; and will do so for most of our branches as we aim to share the same goals with furthering the progress of lazer on our own whims.

@bdach
Copy link
Collaborator

bdach commented May 8, 2023

Ok, I thought that was a git misconfiguration or something. Carry on I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:beatmap-parsing .osu file format parsing realm deals with local realm database size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor does not read properties ignored on BeatmapInfo correctly
4 participants