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

Crash if clicking on any difficulty control point in editor SingleThreaded #9952

Closed
PercyDan54 opened this issue Aug 22, 2020 · 28 comments · Fixed by ppy/osu-framework#3874
Closed
Assignees

Comments

@PercyDan54
Copy link
Contributor

PercyDan54 commented Aug 22, 2020

Describe the crash:
When trying to edit any difficulty control point in any beatmap with SingleThread mode game crashed.
Screenshots or videos showing encountered issue:
屏幕截图 2020-08-22 173721
https://www.youtube.com/watch?v=Ep8vINZxbtA
osu!lazer version:
2020.820.0

Logs:
runtime.log
A part of runtime.log:

2020-08-22 09:36:45 [verbose]: Screen changed → Editor
2020-08-22 09:36:46 [verbose]: updating selection with beatmap:25 ruleset:3
2020-08-22 09:36:52 [verbose]: Unhandled exception has been allowed with 0 more allowable exceptions .
2020-08-22 09:36:52 [error]: An unhandled error has occurred.
2020-08-22 09:36:52 [error]: System.ArgumentOutOfRangeException: Must be greater than 0. (Parameter 'Precision')
2020-08-22 09:36:52 [error]: Actual value was 0.
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.BindableNumber`1.set_Precision(T value)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.BindableNumber`1.BindTo(Bindable`1 them)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.BindableNumberWithCurrent`1.set_Current(Bindable`1 value)
2020-08-22 09:36:52 [error]: at osu.Framework.Graphics.UserInterface.SliderBar`1.set_Current(Bindable`1 value)
2020-08-22 09:36:52 [error]: at osu.Game.Overlays.Settings.SettingsItem`1.set_Bindable(Bindable`1 value) in C:\Users\Administrator\osu\osu.Game\Overlays\Settings\SettingsItem.cs:line 60
2020-08-22 09:36:52 [error]: at osu.Game.Screens.Edit.Timing.DifficultySection.OnControlPointChanged(ValueChangedEvent`1 point) in C:\Users\Administrator\osu\osu.Game\Screens\Edit\Timing\DifficultySection.cs:line 36
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.TriggerValueChange(T previousValue, Bindable`1 source, Boolean propagateToBindings, Boolean bypassChecks)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.set_Value(T value)
2020-08-22 09:36:52 [error]: at osu.Game.Screens.Edit.Timing.Section`1.<LoadComplete>b__19_1(ValueChangedEvent`1 points) in C:\Users\Administrator\osu\osu.Game\Screens\Edit\Timing\Section.cs:line 120
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.TriggerValueChange(T previousValue, Bindable`1 source, Boolean propagateToBindings, Boolean bypassChecks)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.TriggerValueChange(T previousValue, Bindable`1 source, Boolean propagateToBindings, Boolean bypassChecks)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.TriggerValueChange(T previousValue, Bindable`1 source, Boolean propagateToBindings, Boolean bypassChecks)
2020-08-22 09:36:52 [error]: at osu.Framework.Bindables.Bindable`1.set_Value(T value)
2020-08-22 09:36:52 [error]: at osu.Game.Screens.Edit.Timing.ControlPointTable.RowBackground.<>c__DisplayClass7_0.<.ctor>b__0() in C:\Users\Administrator\osu\osu.Game\Screens\Edit\Timing\ControlPointTable.cs:line 189
2020-08-22 09:36:52 [error]: at osu.Framework.Graphics.Containers.ClickableContainer.OnClick(ClickEvent e)
2020-08-22 09:36:52 [error]: at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
2020-08-22 09:36:52 [error]: at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.ButtonEventManager`1.PropagateButtonEvent(IEnumerable`1 drawables, UIEvent e)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.MouseButtonEventManager.handleClick(InputState state, List`1 targets)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.MouseButtonEventManager.HandleButtonUp(InputState state, List`1 targets)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.ButtonEventManager`1.handleButtonUp(InputState state)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.ButtonEventManager`1.HandleButtonStateChange(InputState state, ButtonStateChangeKind kind)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.InputManager.HandleMouseButtonStateChange(ButtonStateChangeEvent`1 e)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.UserInputManager.HandleInputStateChange(InputStateChangeEvent inputStateChange)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.StateChanges.ButtonInput`1.Apply(InputState state, IInputStateChangeHandler handler)
2020-08-22 09:36:52 [error]: at osu.Framework.Input.InputManager.Update()
2020-08-22 09:36:52 [error]: at osu.Framework.Input.PassThroughInputManager.Update()
2020-08-22 09:36:52 [error]: at osu.Framework.Graphics.Drawable.UpdateSubTree()
2020-08-22 09:36:52 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2020-08-22 09:36:52 [error]: at osu.Framework.Platform.GameHost.UpdateFrame()
2020-08-22 09:36:52 [error]: at osu.Framework.Threading.GameThread.ProcessFrame()

Computer Specifications:
CPU i7-9750H
32GB RAM
RTX2070

@peppy
Copy link
Sponsor Member

peppy commented Aug 22, 2020

I'm thoroughly confused. Are you running a custom release? That's not english and we don't have localisation support. Please reports bugs using official releases in the future.

@PercyDan54
Copy link
Contributor Author

Sorry for this.
I'm just using this custom release because it's Chinese.
I'm retrying it in a few minutes with official release

I'm thoroughly confused. Are you running a custom release? That's not english and we don't have localisation support. Please reports bugs using official releases in the future.

@PercyDan54
Copy link
Contributor Author

I'm thoroughly confused. Are you running a custom release? That's not english and we don't have localisation support. Please reports bugs using official releases in the future.

Still crashed
Video:
https://www.youtube.com/watch?v=Ep8vINZxbtA

@peppy
Copy link
Sponsor Member

peppy commented Aug 22, 2020

Weirdly enough, following the same procedure as in your video this doesn't crash here.

Somehow the precision of a bindable somewhere it getting set to zero. I've had a look through the framework-side logic and can't find how this could happen yet.

@bdach
Copy link
Collaborator

bdach commented Aug 22, 2020

A few sanity checks:

  • Is that the only map this occurs on?
  • If it is the only map that crashes, where and when did you get it?
  • Does changing system language change anything?

@PercyDan54
Copy link
Contributor Author

PercyDan54 commented Aug 22, 2020

A few sanity checks:

* Is that the only map this occurs on?

* If it is the only map that crashes, where and when did you get it?

* Does changing system language change anything?
  • No
  • From official website. Tried re-downloading with osu!direct but still crashed
  • I'm using Windows10 Home Single Language which doesn't allow me to change system language

@frenzibyte
Copy link
Member

frenzibyte commented Aug 22, 2020

Can you check if this crash exactly occurs when clicking on any difficulty control point?

i.e. do you get a crash in both of the below procedures:

  1. Download this beatmap and go to editor with it, go to timing section and click on any of the existing difficulty control points, crashed?
  2. Select any beatmap and go to editor with it, add a new control point at whatever time and set it to be a difficulty control point (by checking the Difficulty checkbox at the right side of the screen), crashed?

Or do you get a crash for one but not the other?

@PercyDan54 PercyDan54 changed the title Crash when opening timings in editor Crash when clicking on any timing points with difficulty change in editor Aug 22, 2020
@PercyDan54
Copy link
Contributor Author

PercyDan54 commented Aug 22, 2020

Can you check if this crash exactly occurs when clicking on any difficulty control point?

i.e. do you get a crash in both of the below procedures:

1. Download [this beatmap](https://osu.ppy.sh/beatmapsets/855677#osu/1787848) and go to editor with it, go to timing section and click on any of the existing difficulty control points, crashed?

2. Select any beatmap and go to editor with it, add a new control point at whatever time and set it to be a difficulty control point (by checking the `Difficulty` checkbox at the right side of the screen), crashed?

Or do you get a crash for one but not the other?

Can you check if this crash exactly occurs when clicking on any difficulty control point?

i.e. do you get a crash in both of the below procedures:

1. Download [this beatmap](https://osu.ppy.sh/beatmapsets/855677#osu/1787848) and go to editor with it, go to timing section and click on any of the existing difficulty control points, crashed?

2. Select any beatmap and go to editor with it, add a new control point at whatever time and set it to be a difficulty control point (by checking the `Difficulty` checkbox at the right side of the screen), crashed?

Or do you get a crash for one but not the other?

Clicking the base BPM doesn't crash.
I found the issue:
Only when clicking existed timing points with difficulty it will crash.
The video:
https://www.youtube.com/watch?v=yCVFrjvCvo0
Edit: Tried to add a difficulty control point and saved beatmap.
Then open it again, clicking the difficulty control point I created also crashed
Edit 2:
Noticed this only occurs when I use "SingleThread" as threading mode.
When Multithreaded it doesn't crash anymore

@PercyDan54 PercyDan54 changed the title Crash when clicking on any timing points with difficulty change in editor Crash when clicking on any difficulty control point in editor Aug 23, 2020
@PercyDan54 PercyDan54 changed the title Crash when clicking on any difficulty control point in editor Crash if clicking on any difficulty control point in editor SingleThreaded Aug 23, 2020
@smoogipoo smoogipoo added this to the September 2020 milestone Aug 24, 2020
@frenzibyte
Copy link
Member

frenzibyte commented Aug 24, 2020

Thanks for this information, I can reproduce the issue now (requires setting to single thread mode), will investigate further in a bit.

Interesting thing is that it's only reproducible in a Windows OS, can't reproduce it here on my mac.

@peppy
Copy link
Sponsor Member

peppy commented Aug 24, 2020

Source is this set:

image

@peppy
Copy link
Sponsor Member

peppy commented Aug 24, 2020

@smoogipoo will probably need your feedback on the best path forward here. the editor controls are binding to control points which transfers precision. i'm not sure that's what we want, but the raises the question of whether we should just not be binding in the first place, or setting a precision at bind time?

also this looks like a screwed up precision thing, potentially requiring a framework fix..?

image

@smoogipoo
Copy link
Contributor

That indeed does look like a framework bug. Weird because double.Epsilon > 0 should always be true...

@peppy
Copy link
Sponsor Member

peppy commented Aug 24, 2020

Ignoring that, the more important question is the one above it: how should we go about avoiding the transfer of that legacy precision set?

@smoogipoo
Copy link
Contributor

Maybe either reset the precision (probably not a good idea since you're mangling the timing points immediately) or decouple the slider bars.

@peppy
Copy link
Sponsor Member

peppy commented Aug 24, 2020

The whole point of making control points bindable was for this purpose, so decoupling seems very wrong.

Are we sure legacy needs a precision of epsilon? It's a pity there's no inline documentation as for why that is necessary / examples of where it's used.

@smoogipoo
Copy link
Contributor

Have you tried removing it? We definitely have at least one test case that should fail.

@bdach
Copy link
Collaborator

bdach commented Aug 24, 2020

For what it's worth the epsilon precision does fail a test (LegacyBeatmapDecoderTest.TestDecodeControlPointDifficultyChange) and it was introduced in commit 1a48178. Maybe epsilon itself is too restrictive, it seems to be mostly to prevent using the default precision of 0.1, as the test output is

  Expected: 1.8518518518518519d
  But was:  1.9000000000000001d

@smoogipoo
Copy link
Contributor

It should fail the beatmap tests too, for real world examples:
image

@peppy
Copy link
Sponsor Member

peppy commented Aug 24, 2020

yep, and still fails at 0.01 and 0.001. going to need to think about this one a bit more.

a framework side fix for the precision crash will alleviate the hard crash at least. just means the slider bar will allow more precise setting for legacy points (not such a huge deal).

@bdach
Copy link
Collaborator

bdach commented Aug 31, 2020

this is going to sound silly, but can I get a roll call of the CPU make & model of everyone who has been able to reproduce this?

I'm on an Intel Core i7-6700HQ - interested if everyone else is also replicating this on intel, and if other CPUs (AMD) are also affected

I normally wouldn't go on such a limb but I've spent the evening looking at this with visual studio and the disassembly window and this looks very wrong (this is deep in .NET intrinsics, namely double.CompareTo - had to enable native code debugging to even dig this deep):

image

image

@Joehuu
Copy link
Member

Joehuu commented Aug 31, 2020

Can reproduce on Intel Core i5-4570.

@ColdVolcano
Copy link
Contributor

AMD Ryzen 7 3700X user, able to reproduce

@bdach
Copy link
Collaborator

bdach commented Aug 31, 2020

ok, that's enough to rule this out as vendor-specific, thanks; next best bet is probably some issue with .NET Core's intrinsics. possibly the MXCSR register is misconfigured in one of these cases, but that discussion is probably best not continued here. I'll continue investigating on my own.

@bdach
Copy link
Collaborator

bdach commented Sep 1, 2020

I am almost certain I found what causes the bogus comparison, although I don't have absolutely 100% of the puzzle.

I am 99% sure is that this is indeed related to the MXCSR register value, namely the Denormals Are Zero (DAZ) bit. If it is set, then reads of denormal numbers (including comparisons), which double.Epsilon is, will be coerced to zero, causing the wrong equality result.

As for what causes that, it seems that most likely one of the BASS P/Invokes changes that register's value, and does not restore it to its previous state. In multi-threaded mode this is fine, as the register's value looks to be local to the thread (or the CLR restores it in some manner, not sure), but in single-threaded mode the change pollutes the entire main thread's flags, therefore causing bogus comparisons like the one above. This is unconfirmed, as I can't seem to get windbg to breakpoint conditionally on a register value; I essentially haven't caught the bug red-handed.

However, I tentatively confirmed it in two ways:

  1. Looking at the MXCSR value in the Registers window in Visual Studio in multi-threaded mode - the audio thread gets 0x9FE0, which has bit 6 set, while the rest have variations on the expected value of 0x1F80 (the flag bits naturally vary due to previous FP computaions). Breakpointing on the audio thread and entering double.Epsilon.CompareTo(0) into a watch then returns 0.
  2. Modifying the single-threaded mode slightly so that audio runs on its own thread, while the three remaining threads (input/update/draw) run on the one common thread - it resolves the crash (although it was very slow & janky - likely due to me not doing something 100% properly).

Edit: Here's a definitive video. Tried to step through it in VS's disassembler but something looks real funky. The value changes after a ret (?????).

@smoogipoo
Copy link
Contributor

May be good to shoehorn the pinvoke to BASS_SampleLoad into o!f and see if it still occurs. I'm not entirely convinced it isn't something inside the JIT that's at fault, especially given this code is executing inside a lambda and all that.

If it can still be reproduced with manual pinvokes, then next step would be to inform un4seen (their forum is good).

@bdach
Copy link
Collaborator

bdach commented Sep 2, 2020

I think I can do even better, here's a run-of-the-mill C# console application showing the same thing. Output on my machine is:

epsilon compared to 0 is: 1
epsilon compared to 0 is: 0

The register flags change at the BASS_Init() call.

Edit: Here's the offending instruction.

image

Seems to be coming from somewhere in MSVC.

@smoogipoo
Copy link
Contributor

smoogipoo commented Sep 3, 2020

Good stuff. The MSVC ones look to be functions to set the FP control. So really it's bass+0x1370 or BASS_PluginGetInfo that's not resetting it correctly.

I think this is enough to take it to un4seen.

@bdach
Copy link
Collaborator

bdach commented Sep 3, 2020

Thread is up.

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

Successfully merging a pull request may close this issue.

7 participants