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

Use tagged pointer to represent size/length values #769

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Dec 26, 2024

Objective

This is a pre-requisite for:

As it allows dimension-like values (LengthPercentage, LengthPercentageAuto, Dimension, MinTrackSizingFunction, and MaxTrackSizingFunction) to contain a pointer which can be used to point to a calc() value.

Notes

  • There a small-ish but non-negligible performance hit (around 10-15%) associated with this PR. That's a lot better than previous attempts at calc() and I'm assuming we'll just need to accept this. But if anyone can work out a way to avoid this then that would obviously be great.
  • This requires an MSRV bump all the way up to 1.83 for const f32::to_bits
  • This PR does not implement a Calc representation or the actual Calc resolution. That will be followup work.

Todo

  • Fix 16 failing CSS Grid tests.
  • Fix Yoga benchmark compilation

@nicoburns nicoburns added enhancement New feature or request breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity labels Dec 26, 2024
@nicoburns nicoburns force-pushed the compact-length-repr branch 5 times, most recently from 00600b9 to a3da443 Compare December 26, 2024 05:10
@nicoburns nicoburns marked this pull request as ready for review December 26, 2024 05:20
@alice-i-cecile
Copy link
Collaborator

There a small-ish but non-negligible performance hit (around 10-15%) associated with this PR. That's a lot better than previous attempts at calc() and I'm assuming we'll just need to accept this. But if anyone can work out a way to avoid this then that would obviously be great.

Annoying, but calc doesn't seem to be an optional feature. We could try and feature flag this, but I don't think that users are likely to want to opt out.

This requires an MSRV bump all the way up to 1.83 for const f32::to_bits

Totally fine with this: const floats will be incredibly useful to this crate in general.

This PR does not implement a Calc representation or the actual Calc resolution. That will be followup work.

Good choice!

@nicoburns nicoburns force-pushed the compact-length-repr branch from b2509af to d54147b Compare January 6, 2025 10:21
@nicoburns
Copy link
Collaborator Author

This isn't perfect, but I think I am confident that this is the right direction. So in time-honoured Taffy tradition I think I'm going to merge and then iterate.

@nicoburns nicoburns merged commit afa6437 into DioxusLabs:main Jan 6, 2025
26 checks passed
@nicoburns nicoburns mentioned this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants