Skip to content

fix: Slider put in Canvas NaN Exception#2765

Merged
Eideren merged 1 commit into
stride3d:masterfrom
TranquilAbyss:fix-slider-in-canvas-mouseoverupdate
May 3, 2025
Merged

fix: Slider put in Canvas NaN Exception#2765
Eideren merged 1 commit into
stride3d:masterfrom
TranquilAbyss:fix-slider-in-canvas-mouseoverupdate

Conversation

@TranquilAbyss

Copy link
Copy Markdown
Contributor

Replacing PR #2604 for a more generalize approach as asked.

PR Details

Canvas can provides children with Infinity values for availableSizeWithMargins when measuring. Most UIElements handle Infinity in MeasureOverride. For UIElments that do not handle Infinity, an Infinity check was added to UIElement.Measure as a default behavior to check for Parent.RenderSize recursively in InfinityToParentRenderSize.

This was done recursively in since the Arrange happens after Measure, and Arrange is when RenderSize is normally set except for the root element RenderSize is set earlier.

Note on UIElments Stretch:
I have tested UIElements like Canvas inside of another Canvas and currently the second Canvas size is set to 0. This appears to be caused by childrenDesiredSize being set to NaN then being replace in Measure with a size of 0. The recursion back to the root element in InfinityToParentRenderSize is never reached due to the second Canvas childrenDesiredSize is not set to infinity, so the parent s. Which means that the Stretch behavior is ignored due how the UI currently works. I choose to leave the recursion since it would allow for Stretch to behave as expected, rather than setting values to NaN, then to 0.

Fixing how the code is ignoring UIElement Stretch seemed to me to be scopecreep. I did not want to ignore the structure of the code by having the InfinityToParentRenderSize only look back one parent.

Related Issue

#2236

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.

@Eideren Eideren added the area-UI label May 3, 2025
@Eideren Eideren merged commit 242d414 into stride3d:master May 3, 2025
@Eideren

Eideren commented May 3, 2025

Copy link
Copy Markdown
Collaborator

Thanks !

@Kryptos-FR

Kryptos-FR commented May 3, 2025

Copy link
Copy Markdown
Member

I didn't have time to look at it. Just to be sure I understand, the behavior for canvas was unchanged, right?

If I remember correctly, canvas are by design not supposed to be affected by stretch or most layouting beside setting their position/anchor relative to their parent. That's their point: to give more flexibility to for example draw stuff on top of each other.

See for example the doc for the WPF version: https://learn.microsoft.com/en-us/dotnet/api/system.windows.controls.canvas?view=windowsdesktop-9.0

Canvas is the only panel element that has no inherent layout characteristics. A Canvas has default Height and Width properties of zero, unless it is the child of an element that automatically sizes its child elements. Child elements of a Canvas are never resized, they are just positioned at their designated coordinates. This provides flexibility for situations in which inherent sizing constraints or alignment are not needed or wanted. For cases in which you want child content to be automatically resized and aligned, it is usually best to use a Grid element.

So a second Canvas inside another one having size zero by default is exactly what is expected. When you put elements inside a Canvas, you have to set their size because the Canvas will not do it for you by design.

Looking at the code now, I'm wondering if that doesn't break the assumptions for elements within a Canvas (in which case the recursion should stop when it encounters a Canvas).

@TranquilAbyss

TranquilAbyss commented May 3, 2025

Copy link
Copy Markdown
Contributor Author

The only change made is that after the child determines it size, if the size has an infinity in it then set it to parent RenderSize.

For Canvas in Canvas the recursion currently does nothing and can be removed, since the infinity check logic is not hit since Canvas in Canvas result as NaN size setting the second Canvas size to 0.

I wish I had your comment earlier, since I did not know the goal was to have child objects default to 0 when undefined in a Canvas. The condition of a Slider in a Canvas was a crash, since Infinity was passed as the Size of the slider which turned into NaN a few frames later. So Sliders now use parent RenderSize instead of 0. I am oversimplifying the logic, but you can see the code to see what I mean exactly.

All other UI Element handle infinities within MeasureOverride, so as children they are handled things as they were.

Feel free to message me in discord if you want to discuss this further. If we decide their is a better solution I can implement it. my original PR was just handling the infinity within the Slider measurement. My goal here was to use the "container" size to determine infinity availableSizeWithMargins

@Kryptos-FR

Copy link
Copy Markdown
Member

@TranquilAbyss to be fair, I'm not 100% sure of what I wrote. I just kind of remember that when the UI system was designed, we took inspiration from WPF.

No need to revert anything right now, we can try to make a few samples with complicated UI combining canvas and slider and see if the result look ok.

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.

3 participants