Skip to content

Use actual BeatmapInfo rather than PlayableBeatmap.BeatmapInfo for editor writes#15075

Merged
smoogipoo merged 11 commits into
ppy:masterfrom
peppy:fix-editor-difficulty-name-update
Oct 15, 2021
Merged

Use actual BeatmapInfo rather than PlayableBeatmap.BeatmapInfo for editor writes#15075
smoogipoo merged 11 commits into
ppy:masterfrom
peppy:fix-editor-difficulty-name-update

Conversation

@peppy
Copy link
Copy Markdown
Member

@peppy peppy commented Oct 13, 2021

This will make more sense after IBeatmap.BeatmapInfo is of type IBeatmapInfo. Keep that in mind when reviewing.

Closes #14716 (again).

@peppy peppy added area:beatmap parsing .osu file format parsing area:editor labels Oct 13, 2021
@peppy peppy requested a review from bdach October 13, 2021 05:54
@peppy
Copy link
Copy Markdown
Member Author

peppy commented Oct 13, 2021

I... have no idea how this failed so many times on CI. I can't repro locally, something very weird.

@bdach
Copy link
Copy Markdown
Collaborator

bdach commented Oct 13, 2021

I cannot reproduce the failures, either. Tried adding thread sleeps in a few places too and got nothing for it...

@peppy
Copy link
Copy Markdown
Member Author

peppy commented Oct 14, 2021

I feel like CI just broke in some way. Let's see if a master merge fixes..

@pull-request-size pull-request-size Bot added size/M and removed size/S labels Oct 14, 2021
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Oct 14, 2021
@peppy peppy force-pushed the fix-editor-difficulty-name-update branch from b609212 to 8a4c0c0 Compare October 14, 2021 11:16
@peppy peppy force-pushed the fix-editor-difficulty-name-update branch from 634f55e to ad07324 Compare October 14, 2021 13:13
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Oct 14, 2021
@bdach
Copy link
Copy Markdown
Collaborator

bdach commented Oct 14, 2021

I've confirmed the theoretical I described on discord is indeed the cause of the failures. Essentially the problem is that MetadataSection copies metadata strings into its drawable components and has no way to know if someone mutates BeatmapInfo externally like the test does, and therefore it can drop those external changes if any of the metadata controls lose focus and cause a commit.

The above also explains why waiting for metadata section's LoadComplete() causes the failure to happen 100% (it ensures ordering in what was previously a read race between the section's BDL and the test code).

I have a few immediate ideas on how to fix, the easiest of which would be changing away from the setup screen before modifying BeatmapInfo (confirmed that this fixes). Or using the textboxes rather than manually setting stuff to BeatmapInfo, but that one is surprisingly finicky to get to work because programmatically textbox is broken. Both are arguably hacks but the proper fix would be to eliminate the possibility of such a desync and I'm half hoping we can automatically get that with realm changes.

None of those sound like "transferring single direction" mentioned on discord though, so I'm unsure what was meant by that. As far as I can see the data is already only transferred one direction only after all (namely, out from the metadata screen to the editor beatmap). Maybe that was supposed to read as transferring in both directions?

@peppy
Copy link
Copy Markdown
Member Author

peppy commented Oct 15, 2021

Changing away from the screen sounds best for now. As you mention, updating things for realm will likely fix this in a forceful way.

You can ignore what I said above - I misunderstood what the underlying issue was here.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Oct 15, 2021
@peppy
Copy link
Copy Markdown
Member Author

peppy commented Oct 15, 2021

This should be good to go, finally.

@smoogipoo smoogipoo merged commit 8d11f4c into ppy:master Oct 15, 2021
@peppy peppy deleted the fix-editor-difficulty-name-update branch October 16, 2021 04:31
bdach added a commit to bdach/osu that referenced this pull request Mar 17, 2025
Closes ppy#32420.

The failure cause here is that in editor the beatmap version for the
beatmap affected (or... any beatmap, really), is 0 (ZERO). That is
probably a regression from ppy#32315, but
like... can we universally agree that calling that change "a regression"
in any capacity is dumb? Like what was that code *doing* playing dumb
reference games and copying stuff into an arbitrary instance that could
get or not get used later on? And now you have a 50/50 chance of
accessing the *correct* model's field, depending on whether you go via
`BeatmapInfo` or `Beatmap.BeatmapInfo`?

Moving the field to `IBeatmap`, i.e. what is by now - by consensus,
since ppy#28473 - supposed to be the "decoded
and materialised" beatmap, fixes this issue.

I probably should have done this as part of
ppy#28473 but it slipped my mind. Probably
for the better too because this change has rather large chances of
breaking stuff so maybe better to examine it in isolation (via diffcalc
runs or whatever).

For added humour points, you'd say that the field on `BeatmapInfo` was
not `[Ignore]`d, so this is a realm schema change, right? No. As far as
I can tell, it's not. I opened realm studio and `BeatmapVersion` *is not
a listed column` on `Beatmap` models.

I'm also not gonna get into the fact that I think `EditorBeatmap` doing
dumb games with juggling two `BeatmapInfo` references since
ppy#15075 is bad, because I don't think I
have the mental capacity to hotfix this by going down that train of
thought.
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.

Editor fails to save difficulty name

3 participants