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

Yoga implementation Part 1 #18151

Closed
wants to merge 8 commits into from
Closed

Yoga implementation Part 1 #18151

wants to merge 8 commits into from

Conversation

IceReaper
Copy link
Contributor

@IceReaper IceReaper commented May 19, 2020

For an example what exactly Yoga is, and how it works, see: https://yogalayout.com/ and https://yogalayout.com/docs

Basically all old Bounds stuff is completely replaced by a proper layout engine, which supports basics like margins, borders, padding and min/max and preferred width/height attributes.
Additionally the parent / child relation has to be tied to the yoga library to properly calculate element positions.

Notice:
This PR ensures the old ui yaml files will still work as before. To ensure compatibility, the layout calculation method is way more often called as it should be. These additional calculation calls will be replaced in the future along with the change of the yaml files towards yoga based setups. In the end there should be exactly 1 call to the root widget before drawing the ui. Everything else will be refactored.

Whats not part of this PR:

  • Refactor all the chrome yaml files to make use of the flex layouting engine.

Feature ideas for the future:

  • addition of more css styles for gradients, shadows, border images, font, etc.
  • Implementation of "classes" which can be used to group the style of widgets ".myclass", "myWidgetType", ".myClass .myChildClass" ...
  • Implementation of "pseudo-classes" to implement style overrides for widget states (:hover, :active, ...).

@IceReaper IceReaper force-pushed the yoga branch 2 times, most recently from 9c541ee to b9c0ff5 Compare May 19, 2020 20:12
@pchote
Copy link
Member

pchote commented May 19, 2020

A few more broken things in TD:

  • Dropdown menus
  • Color picker color-watches
  • Lobby player and options bins
  • Button positioning on the "leave mission dialog"

@IceReaper IceReaper force-pushed the yoga branch 4 times, most recently from ba59bab to fbcb9bb Compare May 21, 2020 15:46
@IceReaper
Copy link
Contributor Author

Refactored the visibility part, above broken items should be automatically fixed by that.

@IceReaper
Copy link
Contributor Author

I've included an overhauled mainmenu.yaml relying purely on yoga layouting as an example how future uis will work.

private Widget parent = null;
public Widget Parent { get { return parent; } }
private Func<bool> visibilityFunction;
public Func<bool> VisibilityFunction { get { return visibilityFunction; } set { visibilityFunction = value; IsVisible(); } }
Copy link
Member

Choose a reason for hiding this comment

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

Having this and IsVisible as distinct things feels quite weird IMO, but I don't have any immediate alternatives to suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem here is to allow the old ui to work unaltered. Over time, the old ui will be refactored to rely on yoga features only and we can get rid of those workarounds. In the end this can be cleaned up when we no longer need to provide maximum compatibility with the old setup. The problem is actualy, that Nodes must be visible in order to calculate their layout bounds, which is totaly fine on a pure yoga layout, but as long as we manualy set and directly use bounds for further bounds calculations, i had to hack in several visibility and layouting calls to make sure the old ui properly works. the perfect example is a dropdown, which template widget is not visible, resulting in all clones to not have any bounds. As as conclusion, the end-goal would be to get rid of all the manual calculations, and to refactor the visibility code towards updating the display prop in the layout method.

Copy link
Member

Choose a reason for hiding this comment

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

My expectation/assumption was that clones would copy the string-based definitions (X, Width, etc) and then set their layout based on these values rather than the computed layout of the template. Is there something stopping this from working?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to keep the current IsVisible stuff as it is, but then introduce a new ComputeLayout pass to Widget that evaluates IsVisible to update the yoga visibility and recompute layout before rendering?

@IceReaper IceReaper force-pushed the yoga branch 4 times, most recently from 624de41 to f14f4c2 Compare July 9, 2020 07:51
@pchote
Copy link
Member

pchote commented Oct 3, 2020

I'm wondering whether the solution to making progress here might be to refactor out an IWidget interface, and then create a set of duplicated *YogaWidgets that don't have to worry about maintaining two YAML APIs at once. We can then migrate file-by-file by replacing e.g. Button@FOO with YogaButton@FOO and updating the yaml properties to match.

The duplication isn't nice, but neither are any of the other workarounds that we've brainstormed wrt backwards compat. At least this way we could go straight to the final YAML properties without risking regressions in places that haven't yet been ported, and then simply delete all the old widgets once the migration is complete.

@abcdefg30
Copy link
Member

What is the status here?

@pchote
Copy link
Member

pchote commented Oct 22, 2020

IMO its sad that a lot of @IceReaper's PRs end up sitting idle because they don't get the needed engagement/support from the reviewer side. The last 6 months have been very bad for me, but things are a bit clearer looking forward - if there are no objections then I am thinking I might start with some PRs towards #18151 (comment) during the next release cycle to try and help progress with this feature.

@abcdefg30
Copy link
Member

Closing since there is not much to do in this PR if we want to move forwards with several smaller PRs (and also no activity).

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

Successfully merging this pull request may close these issues.

None yet

4 participants