-
Notifications
You must be signed in to change notification settings - Fork 114
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
Replace per-node measure functions with a single measure function + per-node context #490
Replace per-node measure functions with a single measure function + per-node context #490
Conversation
47a5a3f
to
9865cd1
Compare
a49d152
to
2e037d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, I don't mind this implementation and it seems to unlock a fair bit of use cases.
Because you have to commit to one Context
for everything I was first concerned that it might be harder to create reuseable functionality for this system. But then I'm not sure if reuse across multiple taffy client would even make sense or how that could be implemented differently... so it probably doesn't matter.
I left some minor comments about typos and documentation.
README.md
Outdated
@@ -20,7 +20,7 @@ Right now, it powers: | |||
use taffy::prelude::*; | |||
|
|||
// First create an instance of Taffy | |||
let mut taffy = Taffy::new(); | |||
let mut taffy: Taffy<MeasureFunc<()>> = Taffy::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Context
defaults to ()
, is it possible to just write Taffy<MeasureFunc>
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try it, but I'm guessing not on the basis that Measure
defaults to MeasureFunc<()>
so if that were possible then it ought to be possible to just write Taffy
(but I tried that and it didn't work). I'm thinking maybe an alias like type DefaultTree = Taffy<MeasureFunc<()>>
?
See also: #449 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if we can't get type inference working let's do a type alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My latest thinking on this is that we ought to rename the generic struct to TaffyTree
, and then have a type Taffy = TaffyTree<MeasureFunc<()>>
for backwards compatibility, but otherwise encourage users of Taffy to define their own alias, and documenting a bunch of helpful examples on the TaffyTree
struct.
src/tree/taffy_tree/tree.rs
Outdated
/// Updates the stored layout of the provided `node` and its children | ||
pub fn compute_layout_with_context( | ||
&mut self, | ||
node: NodeId, | ||
available_space: Size<AvailableSpace>, | ||
context: Measure::Context, | ||
) -> Result<(), TaffyError> { | ||
self.context = Some(context); | ||
let result = compute_layout(self, node, available_space); | ||
self.context = None; | ||
result | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this function could cause unintended side-effects because the self.context
is overridden.
Maybe it could make sense to save the old self.context
in a variable and then restore that instead of overwriting with None
?
In any case, I think the behavior with self.context
should be documented in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't, because this is the only place that self.context
is overridden. It's really just a hack to avoid passing the context through all the compute
functions. It shouldn't really work like this though, and your comment has made me realise a different issue with this setup: it's going to require Context
to be Send
/Sync
in order for Taffy
to be Send
/Sync
.
I think the proper fix for this might be another type which wraps Taffy
and Context
and implements LayoutTree
. Last time I tried this kind of things it ended up being a perf regression, but I think I might try it again for this because it would be a lot cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the same conclusion I had when I experimented with this idea, so that's why I was super excited when your approach allowed to preserve the same API surface area (more or less).
But I think fundamentally there needs to be another implementation of LayoutTree
for more a "advanced" use case.
@nicoburns I love the idea! I think it solves some paint points of the sketch I made in #198, but it is spiritually the same what I had in mind. I think your implementation is more elegant :) I'm not sure about the exact API ergonomics, for example pub fn compute_layout_with_context(
&mut self,
node: NodeId,
available_space: Size<AvailableSpace>,
context: Measure::Context,
) -> Result<(), TaffyError> {
self.context = Some(context);
let result = compute_layout(self, node, available_space);
self.context = None;
result
} feels intuitively a bit off, specifically that pattern. self.context = Some(context);
self.context = None; But I don't know on top of my head how it can be structured a bit better. Overall, I think that direction is a big win in my head for the library (as a lib user). Nice job 👍!! |
One other interesting detail/observation: Maybe I'm missing something, but it seems that using the API properly might involve some |
@nicoburns I'm sorry that I'm "climbing into someone else's monastery with my charter" (I'm a C++ programmer, and I only "read" Rust, and don't "write"), but doesn't instantiating a let mut taffy1: Taffy<MeasureFunc<TextLayoutData>> = Taffy::new(); // Instantiation for one type
let mut taffy2: Taffy<MeasureFunc<SomeOtherData>> = Taffy::new(); // Instantiation for another type Two different instances produces 2 unique For example, in C++, not too-safe, but good-old common-pattern for passing any context - using non-owning class SomeSpeciallyMeasuredWidget
{
taffy::NodeId _id;
taffy::Style _style;
SomeUsefulDataForMeasurement _meas_data;
SomeSpeciallyMeasuredWidget (Widget* parent)
: Widget(parent)
{
taffy::Taffy* taffy_tree = this->getRootTree();
// `this` is mutable pointer to self, here is it passed as `void*`
_id = taffy_tree->new_leaf_with_measure_and_context(_style, this,
[](const Size<Option<float>>& known_dimensions, const Size<taffy::AvailableSpace>& available_space, void* context) -> taffy::Size<float>
{
SomeSpeciallyMeasuredWidget* self = static_cast<SomeSpeciallyMeasuredWidget*>(context); // from void* into known type
return my_measurement_computation(known_dimensions, available_space, self->_meas_data);
}
);
}
}; Maybe somehow we can achieve the same in Rust? |
@inobelar I think your intuition is correct, but I'm not sure what is the impact of the issue in practice. E.g. How many different |
@inobelar You're definitely correct that this will generate two copies of everything. And this definitely seems non-ideal given that it's only a very small part of the code that would actually need to change. The Rust equivalent of The other way to avoid duplicating everything would be to make the algorithms take The expected way of using Taffy would be: struct TextLayoutData { ... }
struct SomeOtherData { ... }
enum CombinedData {
Text(TextLayoutData),
Other(SomeOtherData),
}
let mut taffy: Taffy<MeasureFunc<CombinedData>> = Taffy::new(); which also avoids this problem. And I agree with @twop that it would be uncommon to need multiple Taffy instances with different Context parameters. |
f1e12b5
to
22d9fbc
Compare
20f3fd9
to
6e408a6
Compare
I'm building a integration to use taffy for egui for more complex layouts. I based my work on this PR since I hoped it would solve the lifetime issue I was facing, but in the end I had to use some unsafe code to make things work. My api basically looks like this:
let mut taffy = TaffyPass::new(
ui,
Id::new("flexible"),
Style {
display: Display::Flex,
..Default::default()
},
);
taffy.add(
Id::new("child_2"),
Style {
flex_grow: 1.0,
..Default::default()
},
Layout::centered_and_justified(Direction::TopDown),
|ui| {
let _ = ui.button("Button 1");
},
);
taffy.add(
Id::new("child_3"),
Style {
flex_grow: 1.0,
..Default::default()
},
Layout::centered_and_justified(Direction::TopDown),
|ui| {
let _ = ui.button("Button 2");
},
);
taffy.show(); Since egui is immediate mode and I'm creating this as a library to be used in combination with egui's layout capabilities, my use of taffy is probably a bit different than most others. But I think what I'm trying to do should be possible with safe rust. My problem is: The last parameter to the add function is a FnMut closure. I want to pass this as a &mut reference to the measure function and then after the layout was calculated, I want to call it again to create the actual ui. But since the compute_layout_with_context requires the context to live for the length of the Taffy struct, this is not possible. Here is a simple example that fails due to a similar lifetime problem: Details
use taffy::prelude::{Size, Style};
use taffy::tree::MeasureFunc;
use taffy::Taffy;
struct TaffyState<'f> {
taffy: Taffy<MeasureFunc<&'f mut str>>,
}
fn main() {
let mut state = TaffyState {
taffy: Taffy::new(),
};
let node = state
.taffy
.new_with_children(Style { ..Style::default() }, &[])
.unwrap();
{
let mut str = "hello".to_string();
let result = state
.taffy
.compute_layout_with_context(node, Size::length(100.0), &mut str)
.unwrap();
}
} For reference, here is the unsafe code I had to write to make things work. I think what I'm doing there should be sound, but I'm not very experienced with rust safety. Even if this won't be possible for some reason, your PR makes what I'm trying to do much easier, so I really appreciate it! |
Hmm… the lifetime being required to outlive the Taffy struct doesn’t sound right! I suspect this is a result of the hack (temporarily storing the context in the Taffy struct for the duration of the layout) that is currently being used to avoid having to pass the context around. I suspect I’m going to have to update this PR to do things properly, which will likely mean creating a TaffyView struct or similar which can act as an “iterator” over the Taffy tree without actually being the tree. We should be able to store the context in that struct without causing this lifetime issue. I’m quite busy over the next month, so I’m not sure when I’ll next have time to work on Taffy. But this PR is top of my list, and the change discussed above is the main thing blocking this PR from being merged, so hopefully it shouldn’t be too long :) |
@nicoburns , again I apologize for the offtopic, but adding of this new 'generic' parameter also will affect the 'C bindings' #404 and 'Wasm bindings' #394 PRs, or any 'bindings to other languages' in general. Since, we cannot expose generics through 'C ABI' - we need some 'specialization' for that case. Is it will be: |
C and WASM bindings are actually one of the motivations for this PR. It allows them to define their own type to use as the measure function. For C, I am imagining this being something like: struct CMeasureFunc {
context: *mut (),
measure: fn(
known_dimensions: Size<Option<f32>>,
available_space: Size<AvailableSpace>,
context: *mut ()
) -> Size<f32>,
} where For WASM, there would be some type that wraps a handle to a JavaScript function. Notably such functions are |
6e408a6
to
b20cda8
Compare
Co-authored-by: TimJentzsch <[email protected]>
Co-authored-by: TimJentzsch <[email protected]>
… per-node context
e691451
to
dde4431
Compare
@alice-i-cecile Release notes added :) |
Objective
Send+Sync
bounds on measure functions opt-inChanges made
A new parameter (of user-specifiable generic type) has been added to:
Taffy::compute_layout
functionmeasure
function of theMeasurable
trait (measure functions)SyncMeasureFunc
type with Send+Sync bounds has been addedThe parameter passed to the
Taffy::compute_layout
function by the user is passed through when Taffy calls into measure functions.To make this work, an associated type
Context
has been added to theMeasurable
trait, and a generic type parameterMeasure: Measurable
has been added to theTaffy
struct. This generic parameter also allows users of Taffy to choose between the Send+SyncSyncMeasureFunc
and the regularMeasureFunc
which doesn't have those bounds, or to implement their own customMeasurable
type (allowing for things like using an enum instead of a boxed closure).Context
Measureable
type and avoid boxed closuresFeedback wanted
Taffy
more awkward.