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

[Draft] Remove depth from Stride UI #2605

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

MechWarrior99
Copy link
Contributor

PR Details

Removes most Depth related features of the runtime UI system. Including depth based layout, scrolling, and rendering thickness.
Elements can still be offset along the Z axis by setting the new DepthOffset property. Offset is relative to parent element's `DepthOffset.

Related Issue

#2489

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.

Removed Depth related properties from the UI. Switched to using Size2F instead of Vector2/3 when appropriate for better clarity of intent.

Test csprojs contain compile errors still.
Rendering not respecting DepthOffset currently.
@MechWarrior99
Copy link
Contributor Author

MechWarrior99 commented Jan 26, 2025

This is PR is not ready to merge yet.

  • Fix Test compile errors
  • Fix UI not rendering with an offset.

Could use a second set of eyes on the UI rendering, even tested by hard coding it, and I still can't get the boxes to render with a depth offset.

@MechWarrior99
Copy link
Contributor Author

So after a good bit of using the debugger to step through the code I found that the 'issue' with the UI depth rendering is with the ViewProjectionMatrix. Specifically multiplying the matrix because Row3 of it is all 0s.

The ViewProjectionMatrix is ultimately is set from the UIRenderFeature. However ths matrix math is really beyond my understanding of both matricies and rendering/perspective workings. So if somone feels like taking a poke at it that would be great. As I don't even know if Row3 being all 0s is even the real issue.

@TranquilAbyss
Copy link
Contributor

I understand this PR is not ready, But glancing through there a lot of breaking changes in PR. Public variables being deleted, maybe some name changes, Public variable type changes.
I am not recommending leaving dead code, but maybe try

  1. Supporting the public old Vector3 and other 3D types by leaving them to call the new 2D versions
  2. Then deprecating the 3D public vars with [Obsolete] with a message of what replaced them

@MechWarrior99
Copy link
Contributor Author

Thanks for taking a look @TranquilAbyss! I thought about it, but there are a few reasons why I didn't do that. The UI system is pretty much un-used (as far as I can tell), and the when it is used, the positioning related APIs are even less likely to be touched (normally it is like images for items in a inventories and such). So the number of people these breaking changes would effect are very small (again, as far as I can tell). So it didn't seem worth the overhead.
Some are unavoidedable, as otherwise you would be showing either duplicate or dead properties in the the Properties Grid window. Which would just add confusion and bloat.

Plus, by its nature it is a breaking change since it is removing a feature from the UI. My expectation was that the PR would only be merged in for the next major release because of this. So again, less worried about breaking changes.
But if others feel it would be worth it still, I am open to trying to make it less breaking. Not sure a good way to go about it though when taking in to account the Property Grid.

@Eideren
Copy link
Collaborator

Eideren commented Feb 1, 2025

If we were to introduce multiple breaking changes I think it would make sense to bundle it with a couple features. And have at least one more version were those properties are marked as deprecated to notify users

@Eideren Eideren marked this pull request as draft February 1, 2025 19:09
@MechWarrior99
Copy link
Contributor Author

@Eideren Yeah totally makes sense to have some features as well. But wanted to avoid bloating a PR with multiple things. Though that is my tendency haha.

For deprecation, would it maybe make sense to have a version where the only change is marking things with [Obsolete] without any change in behavior? Though not sure how to handle properties like Size that has its type changed.

@Eideren
Copy link
Collaborator

Eideren commented Feb 2, 2025

But wanted to avoid bloating a PR with multiple things

I agree, you can create another PR forked from this branch were you introduce new features, we would merge this one first and the second one with new features soon after

without any change in behavior

Yes, just mention that those still work as is but will be removed in a future version, and point to an existing stable alternative between the two versions if there is one.

Though not sure how to handle properties like Size that has its type changed.

We can't do much in that regard

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

Successfully merging this pull request may close these issues.

3 participants