fix: Presentation, remove false-positive prone exception

[Eideren]

PR Details

See #2427 for the basics.
I did not change it into an assert as this would block execution every time someone change properties in the UI editor with a debugger attached, making for a poor experience.
Lastly, I could not find any straightforward path to specialize this check to avoid false positives as this scope is too low level.
Every time this one triggered for me was for a false positive, given that, I believe removing it altogether is preferable.

Related Issue

Fix: #2427

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[Kryptos-FR]

Historically it was added to figure out hard-to-detect bugs especially regarding the property grid and the graph/node view models.

Instead of crashing in OnPropertyChanged, we could break the debugger (if already attached). That way only debugging sessions are affected, and one can easily comment out that line if necessary.

[Eideren]

Changing properties in the UI editor triggers this codepath, changing some properties in user scripts do too, the two of them are false positives. Given how prevalent this is, if we expect users to comment out the assert, I feel like it kind of defeats the purpose of having the assert in the first place, no ? Shouldn't we do it the other way around, only enable it on our side if we ever fall in a case where this could help given that the latter is exceptionally rare in comparison ?

I understand that it would be preferable for us to have this case covered, but users are bound to come in and comment on this assert every time they fall onto it, or worse, they keep it in just in case as well and become increasingly frustrated every time they manipulate properties.

[Kryptos-FR]

It only happens in debug. Official release doesn't have debug. It was never meant to be found by users but to be used internally.

Users compiling the engine themselves should also do it in release mode. Only people working on the engine or editor should compile in debug mode. And we want to find those bugs in such case.

I could be wrong but I seem to remember that structs have been supported in the property grid for a long time. Does the same bug happens in earlier version of Stride (or even Xenko)? It could be the symptom of a regression.

[Eideren]

Users do run the engine in debug, it is the default mode when following the instructions to build the engine, and we tend to have more users building the engine locally than other engines.

It's not just writing to structs, the field needs to be inside a container for the assert to occur as I mention in the issue linked. This issue has been part of the engine since 3.1.0.1-beta02 as you can see in the duplicate to the linked issue.

Let's say we change it to an assert, is that actually going to be useful for debugging when all it has done as of now is report false positives - it seems more likely to do the opposite, have someone waste their time looking into that assert when it's actually working as intended. We should either rework the check to avoid false positives, or remove it entirely.

[xen2]

Agree, if it happens and bother normal execution, it should be gone.
Let's add more specific checks as necessary on future situations (if/when they arise).