🎋 Fix #11: Id clash when using multiple Trees
#12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tosti007
force-pushed
the
fix-multi-tree-id-clash
branch
from
e91f5a3 to
f5662bf
Compare
hannes-vernooij
approved these changes
Jul 25, 2023
38 tasks
|
Ran into this issue today - I've pulled that PR in my fork - works. Thanks :) |
|
I'll rebase this once #13 lands, unless someone needs this sooner, if so let me know! |
tosti007
added a commit
to Traverse-Research/egui_tiles
that referenced
this pull request
Sep 28, 2023
tosti007
force-pushed
the
fix-multi-tree-id-clash
branch
from
f5662bf to
ed81533
Compare
tosti007
added a commit
to Traverse-Research/egui_tiles
that referenced
this pull request
Nov 2, 2023
emilk
pushed a commit
that referenced
this pull request
Nov 2, 2023
This PR marks `serde` as an option dependency and only derives the various `Serialize` and `Deserialize` traits when this feature is enabled. Regardless of not forcing the user to have the `serde` dependency, having the `serde` feature of `egui` (optionally) enabled is a requirement for #12.
tosti007
force-pushed
the
fix-multi-tree-id-clash
branch
from
ed81533 to
67b4cff
Compare
tosti007
added a commit
to Traverse-Research/egui_tiles
that referenced
this pull request
Nov 3, 2023
emilk
approved these changes
Nov 10, 2023
|
I'm finishing this up in #32 |
emilk
reviewed
Nov 21, 2023
Comment on lines
+175
to
+176
| self.id = ui.id(); | ||
|
|
There was a problem hiding this comment.
I looked at this wrongly you're right!
|
Sorry, this slipped my mind! I thought I enabled you to push to this branch, but fixing it in #32 is fine! |
emilk
added a commit
that referenced
this pull request
Nov 21, 2023
* Closes #11 * (this is a fork of #12) --- Each `Tree` now has a globally unique id, meaning the same `Tree` can be rendered in different `Ui`:s and still work, and you can have multiple trees. However, it is up to the user to pick a globally unique id. --------- Co-authored-by: tosti007 <git@brianjanssen.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #13 (will update and rebase once that's in), to have the
egui/serdedependency enabled.This PR is an implementation to solve issue #11.
This is achieved by storing an
egui::Idfor each tree. In order to get theTiledId::egui_idtheTree's id is passed. The value of theTree'sidis determined by a user passed value, thus this is a breaking change.I opted to allow
serdeto serialize and deserialize the passed value, as adding aid_sourcekind of function, which needs to be called after deserialization felt bad. I also opted to have the user pass anInto<egui::Id>rather than obtaining it from theegui::Uipassed in theTree::uifunction, as that will cause theTileIds to break when the user draws the sameTreewithin a differentUicontext.