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

Add pixel ratio config, document Style/Layout as logical/physical pixels #478

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SimonSapin
Copy link
Contributor

Objective

This PR represents the outcome of discussions around how different length units should be interpreted when parsing CSS syntax, in particular #460 (comment). The doc-comment for set_pixel_ratio in src/tree/taffy_tree/tree.rs details the proposed design.

Context

Previous discussion happened in #440 and #460

Feedback wanted

Documentation and API changes are in this first PR draft, but behavior changes (actually applying the pixel ratio) are not implemented yet. The (style) inputs and and eventual (layout) outputs are to use different units, but at what point should the conversion be done? The reduce bug risk I think intermediate computation (including some public APIs like SizeAndBaselines, MeasureFunc, and likely others) should all pick one unit or the other but avoid dealing with both at the same time, with conversion kept in a small number of convenience functions or methods.

  • If computation works with physical pixels, conversion should happen when it accesses values from Style. Perhaps in the same APIs that resolve percentages. A downside is that a &Taffy or &TaffyConfig parameter will need to be added to various functions and methods, and all of their call sites
  • If computation works with logical pixels, conversion should happen at the very end just before rounding. Either when creating each Layout value, or mutating them like rounding does. The latter muddies how we define the meaning of that struct, while the former breaks things if Layout ever used as an intermediate result for further computation. Is that the case?

@nicoburns nicoburns added the enhancement New feature or request label May 16, 2023
@nicoburns
Copy link
Collaborator

This looks good so far. Regarding the conversion point, I think we ought to convert when writing to Layout. There are 3 places where the layout is read:

  1. When performing rounding the unrounded value is (obviously) read
  2. The taffy::util::debug::print_tree function reads the values to print them
  3. The CSS Grid algorithm reads from it in one place (https://github.com/DioxusLabs/taffy/blob/main/src/compute/grid/mod.rs#L539) when the first baseline of the grid container.

I think 1 and 2 are fine. 3 would be a problem, but it can easily be eliminated (by storing the unconverted (logical) computed y position of item in the GridItem struct during final positioning and reading it from there instead).

One potential option is to convert the layout_mut function (that returns a mutable reference) into a set_layout function (that takes the new layout as an argument). If we did that, then we could potentially have the conversion logic in a single place within that function (the algorithms would pass logical units into set_layout and set_layout would convert them to physical units before writing them).

@SimonSapin
Copy link
Contributor Author

This is getting farther from my original motivation in #440 of showing how to use cssparser (which isn’t really documented, sorry about that). I’m unlikely to do further work on this PR, so I’d appreciate if someone wants to take over.

@alice-i-cecile alice-i-cecile added the adopt me The author has vanished or is too busy; feel free to create a new PR with the changes! label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adopt me The author has vanished or is too busy; feel free to create a new PR with the changes! enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants