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

[Merged by Bors] - depend on dioxus(and bevy)-maintained fork of stretch (taffy) #4716

Closed
wants to merge 15 commits into from

Conversation

colepoirier
Copy link
Contributor

@colepoirier colepoirier commented May 10, 2022

Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency stretch. Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github here (taffy on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.


Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes #677

Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.

@colepoirier
Copy link
Contributor Author

colepoirier commented May 10, 2022

Bevy UI Examples Screenshots

Here are the screenshots for each of the four bevy ui examples showing that the layout is broken by the changes in this PR.

text_debug

main

main_example_text_debug

this PR

stretch2_example_text_debug

text

main

main_example_text

this PR

stretch2_example_text

button

main

main_example_button

this PR

stretch2_example_button

ui

main

main_example_ui

this PR

stretch2_example_ui

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR C-Bug An unexpected or incorrect behavior C-Dependencies A change to the crates that Bevy depends on labels May 10, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 10, 2022

Those breaks are pretty rough :( We're going to need a much better test suite to fix this.

@rparrett
Copy link
Contributor

rparrett commented May 10, 2022

It would be really helpful if Dioxus maintained a changelog. It seems like the "24 commits ahead" are:

So just a couple of layout bug fixes and davier's partial perf fix so far.

Are bevy's examples relying on buggy layout behavior, or are these fixes introducing new bugs? Or has the migration to stretch2 gone awry in some way?

@colepoirier
Copy link
Contributor Author

It would be really helpful if Dioxus maintained a changelog. It seems like the "24 commits ahead" are:

* Renaming the project

* [Jk/nested fix DioxusLabs/stretch#1](https://github.com/DioxusLabs/stretch/pull/1)

* [fix re-computing layout moves children of non-zero positioned parent DioxusLabs/stretch#2](https://github.com/DioxusLabs/stretch/pull/2)

So just a couple of layout bug fixes so far.

Are bevy's examples relying on buggy layout behavior, or are these fixes introducing new bugs? Or has the migration to stretch2 gone awry in some way?

All excellent points @rparrett!

@colepoirier
Copy link
Contributor Author

Jk/nested fix DioxusLabs/stretch#1 includes 17 commits FYI

@rparrett
Copy link
Contributor

Jk/nested fix DioxusLabs/stretch#1 includes 17 commits FYI

Thanks, updated summary in my comment.

@Sheepyhead
Copy link
Contributor

I tested this PR with my most complicated piece of bevy_ui code; an inventory and equipment half-window. Here is what it looks like on Bevy main at time of writing:
image
And the following is what it looks like when I depend on this PR's changes:
image
The code has been extracted from a game I've been working on and is largely unchanged except for the sake of brevity and cutting out all game-specific mechanics, you can see the repo here https://github.com/Sheepyhead/bevy_ui_test

@mockersf
Copy link
Member

Stretch used to give relative layout for nodes, now it's the absolute layout. I believe it's an issue on Stretch side and opened DioxusLabs/taffy#6, but it's not complicated to fix on Bevy side if they want to stick to the new behaviour.

@colepoirier
Copy link
Contributor Author

Stretch used to give relative layout for nodes, now it's the absolute layout. I believe it's an issue on Stretch side and opened DioxusLabs/stretch#6, but it's not complicated to fix on Bevy side if they want to stick to the new behaviour.

Tested the bevy ui and button examples using François' PR fixing relative positioning and it looks like it fixes it.

ui

mockersf_stretch2_example_ui

button

mockersf_stretch2_example_button

@Sheepyhead
Copy link
Contributor

I can confirm that using Francois' latest changes on this PR completely fixes the layout of my complicated gui
image

@alice-i-cecile
Copy link
Member

Excellent; I'll see about cutting a release of stretch2 ASAP then.

@colepoirier
Copy link
Contributor Author

colepoirier commented May 25, 2022

The two UI examples look correct using stretch2 v0.4.3.

ui

stretch2_0 4 3_example_ui

button

stretch2_0 4 3_example_button

@mockersf Are there any further steps I need to take with this PR?

Edit: It looks like the check-bans CI is still failing on something to do with the lockfile that I don't understand. Do you know what is happening here and if I need to do something to resolve it?

@mockersf
Copy link
Member

@mockersf Are there any further steps I need to take with this PR?

I think @alice-i-cecile intend to release the crate renamed soon, maybe wait for it

@mockersf
Copy link
Member

Edit: It looks like the check-bans CI is still failing on something to do with the lockfile that I don't understand. Do you know what is happening here and if I need to do something to resolve it?

It seems the new crate has a few dependencies in multiple versions which we don't allow in Bevy. You can investigate on the dependencies if the duplicates are really needed, then open PRs on the dependencies to update if possible... It can take a lot of time

@alice-i-cecile
Copy link
Member

You can investigate on the dependencies if the duplicates are really needed, then open PRs on the dependencies to update if possible... It can take a lot of time

I will happily accept any PRs to do this for sprawl. I'll bump the version of the dependencies where I can for the initial release of sprawl as well.

@Weibye
Copy link
Contributor

Weibye commented Jun 6, 2022

It seems the new crate has a few dependencies in multiple versions which we don't allow in Bevy. You can investigate on the dependencies if the duplicates are really needed, then open PRs on the dependencies to update if possible... It can take a lot of time

Seems all dependency issues start with heapless (direct dependency in sprawl) and it's dependency tree

@colepoirier
Copy link
Contributor Author

It seems the new crate has a few dependencies in multiple versions which we don't allow in Bevy. You can investigate on the dependencies if the duplicates are really needed, then open PRs on the dependencies to update if possible... It can take a lot of time

Seems all dependency issues start with heapless (direct dependency in sprawl) and it's dependency tree

I'm not sure how to fix this. Do you have any suggestions?

@alice-i-cecile
Copy link
Member

@colepoirier can you make a PR to sprawl removing heapless? I don't think it's pulling its weight, and I would like to simplify that section of the code anyways.

@colepoirier colepoirier changed the title depend on dioxus-maintained fork of stretch (stretch2) depend on dioxus-maintained fork of stretch (sprawl) Jun 7, 2022
@colepoirier
Copy link
Contributor Author

@colepoirier can you make a PR to sprawl removing heapless? I don't think it's pulling its weight, and I would like to simplify that section of the code anyways.

@alice-i-cecile switched from stretch2 v0.4.3 to sprawl git main to check if that resolves the CI check-bans failures. Regardless I will attempt to remove heapless from sprawl and make a PR. Additionally, the sprawl v 0.1.0 release seems to be missing the vast majority of the contents of the library so will need a v 0.2.0 before we can take a dependency on it for bevy.

@alice-i-cecile
Copy link
Member

@colepoirier the PR title and PR description need to be updated too.

@colepoirier colepoirier changed the title depend on dioxus-maintained fork of stretch (sprawl) depend on dioxus-maintained fork of stretch (taffy) Jun 10, 2022
@colepoirier
Copy link
Contributor Author

@colepoirier the PR title and PR description need to be updated too.

I rewrote it thanks for the heads up! @alice-i-cecile what should I do about the removal of Direction from taffy in bevy_ui? What I did is just delete the code that used to be impl From<Direction> for stretch::style::Direction. Is that the right thing to do? Is Direction something that will need to be added back to bevy_ui after it is re-implemented in taffy?

@alice-i-cecile
Copy link
Member

Direction in this sense refers to text direction. IMO it's out of scope for a layout library. Bevy should implement its own type when it needs this.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

That was about as painless as I could have hoped. @colepoirier have you verified that the UI examples all look okay with the newest changes?

@colepoirier
Copy link
Contributor Author

That was about as painless as I could have hoped. @colepoirier have you verified that the UI examples all look okay with the newest changes?

Yes both look identical to the screenshots above for the bevy_ui on main.

ui

taffy_example_button

button

taffy_example_ui

@colepoirier
Copy link
Contributor Author

FYI I also removed the section for stretch in the deny.toml. I think that lies within the scope of this PR but I'd like confirmation. Please let me know if that is correct.

@TimJentzsch
Copy link
Contributor

Direction in this sense refers to text direction. IMO it's out of scope for a layout library. Bevy should implement its own type when it needs this.

Since the flexbox algorithm depends on writing direction to determine start and end we will probably need to add a way to provide this again.

@mockersf
Copy link
Member

just tried a complex case that was running at 45 fps with stretch, I get 160 fps with this PR 👍

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jun 11, 2022
@colepoirier colepoirier changed the title depend on dioxus-maintained fork of stretch (taffy) depend on dioxus(and bevy)-maintained fork of stretch (taffy) Jun 11, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Looks good to me! The Bevy side was an easy review. Couldn't find any regressions. Did a taffy/stretch diff review and the changes look good to me.

I'm excited for our new future of collaborative UI layout dev!

@cart
Copy link
Member

cart commented Jun 21, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jun 21, 2022
# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes #677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
@bors bors bot changed the title depend on dioxus(and bevy)-maintained fork of stretch (taffy) [Merged by Bors] - depend on dioxus(and bevy)-maintained fork of stretch (taffy) Jun 21, 2022
@bors bors bot closed this Jun 21, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 22, 2022
…gine#4716)

# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes bevyengine#677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 22, 2022
…gine#4716)

# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes bevyengine#677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…gine#4716)

# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes bevyengine#677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…gine#4716)

# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes bevyengine#677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…gine#4716)

# Objective

DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0.

## Solution

I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about.

---

## Changelog

Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch).

fixes bevyengine#677

## Migration Guide

The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Dependencies A change to the crates that Bevy depends on X-Controversial There is active debate or serious implications around merging this PR
Development

Successfully merging this pull request may close these issues.

very low performance when spawning UI nodes one inside the other
8 participants