Skip to content

Conversation

@chris-ehmann
Copy link
Contributor

Fixes #34242, fixes #26001

The first issue is caused by the cancellation token being cancelled on subsequent background changes and then being reused, which then cancels the refresh from happening. I've simply modified to create a new source on each refresh.

The second issue is caused because when we pass IBindable<WorkingBeatmap> Beatmap.Value into EditorBackgroundScreen, the background is null and stays null even when the background has been changed, so we just always fall back to the default background when a refresh occurs. I've changed EditorBackgroundScreen to instead take in IBindable<WorkingBeatmap> rather than WorkingBeatmap, which seems like the most straightforward fix?

@peppy peppy requested a review from bdach July 21, 2025 05:53
private IFrameBasedClock? clockSource;

public EditorBackgroundScreen(WorkingBeatmap beatmap)
public EditorBackgroundScreen(IBindable<WorkingBeatmap> beatmap)
Copy link
Collaborator

@bdach bdach Jul 21, 2025

Choose a reason for hiding this comment

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

Please explain this part of the change because it makes zero sense to me. The cancellation token part of this makes sense. This does not.

You have not changed anything about the mechanics of reading from this. The timing of the reads is unchanged. You're not subscribing to this bindable or anything. Moreover I cannot even see any effect of this change, just the cancellation token part seems to fix the issue.

So I see two possibilities here:

  • You saw something that wasn't there / got confused about this part of the issue, and this part of the change does nothing and should be reverted
  • In certain scenarios that you haven't described in appropriate detail this does make some difference, which is scarier because that should not be the case! As in, it could be evidence of some async off-thread process which means this code is probably unsafe out the wazoo anyway and needs to be properly rewritten rather than playing races with something else.

Copy link
Contributor Author

@chris-ehmann chris-ehmann Jul 21, 2025

Choose a reason for hiding this comment

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

Sorry, I didn't properly describe the issue that this is addressing. #26001 refers to the fact that when you create a new beatmap from the main menu screen, the background does not change at all (atleast, until you exit and re-enter the editor). This has nothing to do with the cancellation token change. I've recorded a demonstration below to show what the current behaviour (master) is. Note that I've applied the cancellation token change (as can be seen in the later half of the video) to demonstrate it has no effect on this issue.

Screencast.From.2025-07-21.01-55-59.mp4

From my very minor testing, what I can identify is that the copy of WorkingBeatmap that's passed into EditorBackgroundScreen does not update the value of the background whenever it's updated in ResourcesSection. The metadata for the background remains null, GetBackground() always returns null, etc. It seems as if this is being copied by value rather than reference in this specific scenario, but this may just be a lack of knowledge on my end. This is why I changed it to take in IBindable<WorkingBeatmap> directly, rather than the value.

Copy link
Collaborator

@bdach bdach Jul 21, 2025

Choose a reason for hiding this comment

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

Well okay that explanation would have been rather helpful to begin with. And saved me time.

The bear trap in question here is

Schedule(() =>
{
// we need to avoid changing the beatmap from an asynchronous load thread. it can potentially cause weirdness including crashes.
// this assumes that nothing during the rest of this load() method is accessing Beatmap.Value (loadableBeatmap should be preferred).
// generally this is quite safe, as the actual load of editor content comes after menuBar.Mode.ValueChanged is fired in its own LoadComplete.
Beatmap.Value = loadableBeatmap;
workingBeatmapUpdated = true;
});

which ends up not being quite true for the background in this specific scenario because the background screen is likely to read Beatmap.Value before the above code runs which materially only matters in the new beatmap case.

Given that the metadata section is signaling the background to refresh manually the idea is probably fine (high emphasis on "probably"), but the execution is not. Bindables are more or less "owned" by drawables. Passing one via constructor is a Bad Idea and can lead to much breakage (memory leaks, spurious undesirable unbinds, etc.)

Please resolve IBindable<WorkingBeatmap> using DI instead. (And add inline comments explaining as to why it's being resolved rather than passed, probably.)

Alternatively EditorBackgroundScreen should have a plain setter for the working beatmap but I'm not sure whether the code complexity that will result from this will be worth the trade-off of "absolute correctness".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please resolve IBindable<WorkingBeatmap> using DI instead. (And add inline comments explaining as to why it's being resolved rather than passed, probably.)

aeb1941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants