Add feature to use percentage as value unit #258
Assignees
DaddyFrosty
Clear assignees
DaddyFrosty
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: DaddyFrosty/yoga#258
Reference in New Issue
Block a user
No description provided.
Delete Branch "percentage-feature"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds the feature to use percentage as a value unit.
You can use the function
YGPx(float)andYGPercent(float)for convenience.I did some benchmarks:
It is slightly faster, as I was able to reduce the calls to
isnan().This is really cool! I'm worried about perf, we need to benchmark this on low end android phones to get a sense of what the impact is. In a perfect world perf should only be effected when using percentage values. Percentage values are quite rare so impacting perf negatively for the rest of use cases is not great.
Some things I would like to see:
Looking forward to working together on this.
Prefix them with
CSS.As I have non available in near future, could someone please try this on a low end device?
Lot's of use cases can be solved via "spacer" views. But in my opinion this leads to less readable code on these places, as the intent isn't that clear anymore.
Additionally if you try to position an element absolute with percentage (e.g
top: 20%,left: 10%) this shouldn't be possible at all, without having to creating a spacer view grid.One use case I have is to layout boxes with wrapping, where I want to have two elements in a row. I would express this as
FlexShrink: 0, FlexGrow: 0, FlexBasis: 50%.But my personal prefered use case is to make my view code more readable and maintainable.
Will do
This is the start for units,
emshould be pretty easy to add (even so this can be calculated manually by now)not sure if this is possible without doing a big refactoring. But I'll have a look.
Would love concrete examples of what views benefit from this, not description of what it could be used for. I'm not really questioning the usefulness of it but it would be great to get a sense of the impact this has. In practice what layout problems does it solve. I myself find it very seldom that I would like to position something a percentage from the top/left as this generally does not scale very nicely. If there is a performance hit due to adding units I want to be able to show all the concrete benefits it brings.
I can make sure to run some tests on perf on android. I won't have much time in the coming week though.
Sorry for bringing up a fictional example. The only real use case I had is the wrapping stuff, where I currently have to reimplement the wrapping logic, by boxing each line in its own container and distribute the elements across it. But I'm not able to express a minimal width on the containig elements, which I would like to.
e.g:
for a width with >= 100px you have two elements per row and < 100px you have a single element per row.
@woehrl01 Thanks for the update!
@woehrl01 updated the pull request - view changes
Ups, got it even faster than without this feature 😆 (same maschine), still interested in how this performs on android.
@woehrl01 awesome! I landed a diff yesterday for experiment support in gentest. I would suggest you focus on added a bunch of test cases now 👍
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@emilsjolander Merged master branch. And got the first failing test. Will look into this further in the next days.
@emilsjolander the failing test is not introduced via this implementation. created a new issue #261 for tracking this.
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
Please change enums.py and re-generate the enum files by running
python enums.py👍I think once this is rebased it will be pretty close to mergable
@emilsjolander sadly not. I already did a local rebase, but I found a test layout which doesn't get calculated correctly. Still want to find and fix that bug before I'll update the PR.
@woehrl01 Ok! Feel free to update the PR with your progress and I can help debug if you want.
@woehrl01 updated the pull request - view changes
@emilsjolander I would highly appreciate if you could take a look! I added some additional variables for easier debugging, I will remove them as soon as we could track down the bug.
@emilsjolander I have a trace, we need to calculate the padding/margin percentage only based on the parents width. see https://www.w3.org/TR/CSS2/box.html#margin-properties
sorry for letting this slipping through. 😞
wait.. margin-top: 50% is 50% of the width?? this makes no sense.
No, its 50% of the parents width. This SO answer explains it best why: http://stackoverflow.com/a/18295172
Couldn't believe this either at first 😄
Oh wow that is super confusing.
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@emilsjolander test is passing 👍 😄
This is starting to look very promising.
Could you
Also i'm not sure about the best way on constructing these values.
YGPxetc is pretty verbose but i'm also not a fan of exposing a global unprefixedpxfunction.@woehrl01 updated the pull request - view changes
@@ -0,0 +1,82 @@<div id="percentage_width_height" experiments="Rounding" style="width: 200px; height: 200px; flex-direction: row;">Indentation here seems a bit off. Could you please update your tests to look more like the others?
Still missing tests for % border values
added those tests
there is no need to write tests for border, as border can't have percentage in css.
I already added two function e.g.
YGNodeStyleSetWidthWithUnitand one onlyYGNodeStyleSetWidthwhich takes onlyfloatand means pixel.My idea was to use
*WithUnitin any interop code, it maybe should be used with an enum instead of the YGPx, e.g.YGNodeStyleSetWidthWithUnit(root, 14, YGUnitPercent). And to keep the "old" for not having a breaking change and to be able to set those values less verbose onCcode.For C# I'd suggest having a struct with implicit convertion from
floatand an extension method to make something like:For Java you can use overloads
@@ -0,0 +1,82 @@<div id="percentage_width_height" experiments="Rounding" style="width: 200px; height: 200px; flex-direction: row;">there is no need to write tests for border, as border can't have percentage in css.
@woehrl01 updated the pull request - view changes
So consistent.
Didn't see that, cool. It's pretty verbose and ads a lot of functions to the API. I think the API for this is worth thinking a bit about before merging, I'll try to see if I can figure out something better. Feel free to post more suggestions as well.
This seems super magical to me. But if that is the standard why for doing such things in C# then by all means. I would probably prefer an enum in both java and c#.
** Travis is failing **
@woehrl01 updated the pull request - view changes
Regarding C:
I think we should have multiple functions to not break many users and still make this opt-in for the ones who want to use percentage. But maybe as a second layer?
YGNodeStyleSetWidthis callingYGNodeStyleSetWidthWithUnitin a thin wrapper? Not sure if this overhead is worth it.Regarding C#:
Was just an idea, could also be something like:
If you use an enum for C# you miss the change to use Properties, also there is this
YogaNode.Create(). I would personaly still prefere the "object initializer syntax":@woehrl01 updated the pull request - view changes
I'll finish up reviewing the algorithm + test changes later today / early next week.
I like that you are thinking about not breaking the API but in this case i'm holding off on marking a stable release for changes such as this one. Also very few projects depend directly on the C version and instead use the wrappers. As I said I think it is worth taking some time and thinking about this API.
Makes sense.
Travis is still failing, because of the not exposed possibility of percentage in Java, I expect for C# to have the same problem.
Take your time reviewing and thinking about a good API. As soon as we have a well-conceived idea I'm happy to make the required changes.
@@ -21,6 +21,13 @@ typedef enum YGFlexDirection {YGFlexDirectionRowReverse,YGUnitModeCount?? should be YGUnitCount probably. Did you forget to re-generate after changes?
return YGValue {
.value = ...,
.defined = ...,
.unit = ...,
};
YGValueToFloatis not very descriptive of what the function does. I would preferYGValueResolveor something similar. What do you think?Create a
YGValueUndefinedstatic instance which you just copy here and below.return YGValue { ... };
return YGValue { ... };
@@ -504,1 +600,4 @@return YGFloatIsUndefined(b);}return fabs(a - b) < 0.0001f;}just check for
.defined. this function just makes it more verbose.@@ -1193,3 +1287,3 @@YGNodeMarginForAxis(child, YGFlexDirectionColumn);YGNodeMarginForAxis(child, YGFlexDirectionColumn, width);}Position percentages are calculated based on the same axis as the position (https://jsfiddle.net/jckmwztt/15/) this is different from margin and padding and instead similar to width and height. So this (and other locations) should be
YGFlexDirectionColumn, height@@ -38,6 +38,11 @@ typedef struct YGSize {float height;remove empty line
space before { (actually just run format.sh)
@@ -85,3 +90,3 @@WIN_EXPORT bool YGValueIsUndefined(const float value);WIN_EXPORT bool YGFloatIsUndefined(const float value);No reason to have this function as we export the
definedmember ofYGValue@@ -110,1 +115,4 @@#define YG_NODE_STYLE_PROPERTY_UNIT(type, name, paramName) \WIN_EXPORT void YGNodeStyleSet##name(const YGNodeRef node, const float paramName); \WIN_EXPORT void YGNodeStyleSet##name##Percent(const YGNodeRef node, const float paramName); \Let's remove the *WithUnit APIs and just use units everywhere. This makes the API smaller and more concise. Also it forces the user to think about units.
@@ -110,1 +115,4 @@#define YG_NODE_STYLE_PROPERTY_UNIT(type, name, paramName) \WIN_EXPORT void YGNodeStyleSet##name(const YGNodeRef node, const float paramName); \WIN_EXPORT void YGNodeStyleSet##name##Percent(const YGNodeRef node, const float paramName); \How should we return the units in the getter? Should I use YGValue in both get + set or use (float, YGUnitPixel) in the set and return only the float in get?
Please don't let me use this syntax here and at the other spots. Visual Studio (at least VS13 which I use) doesn't recognize this syntax, which would need me to change this one everytime I need to compile. (thats the reason I missed some tests at first)
sounds good!
you mean a "global" instance? I could use this in
YGComputedEdgeValue, too.@@ -1193,3 +1287,3 @@YGNodeMarginForAxis(child, YGFlexDirectionColumn);YGNodeMarginForAxis(child, YGFlexDirectionColumn, width);}nice catch, thanks!
@@ -110,1 +115,4 @@#define YG_NODE_STYLE_PROPERTY_UNIT(type, name, paramName) \WIN_EXPORT void YGNodeStyleSet##name(const YGNodeRef node, const float paramName); \WIN_EXPORT void YGNodeStyleSet##name##Percent(const YGNodeRef node, const float paramName); \The getter should return a YGValue as well.
VS15 has support for this syntax and it is used throughout the project so i suggest upgrading. I would prefer it and it is part of C99. Is there a reason you are using a old VS build? if there are legit usages for old VS then I would be OK with leaving as is.
yes, easy to mix up terminology when working with many languages 😕
none, except licence. If VS15 supports this I'm fine, making this change.
I think it does from searching google :p please double check yourself
@woehrl01 updated the pull request - view changes
sorry, just downloaded VS15 community and I can't get it to compile with this syntax 😞
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@@ -38,6 +38,11 @@ typedef struct YGSize {float height;I would like to do this, but setting everything up on my maschine would be a huge job. As I have no linux runtime currently available, and also no clang localy. It would be greate if you could run this, after your import, if you don't mind. Same for enum.py, currently I have no python installed. If not I'll try my best to set this up locally, could take a while (the clang environment).
@woehrl01 updated the pull request - view changes
ups, got it to work, just can't return it at the same time. 😕
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
Looking very good. Two tiny changes just
@@ -38,6 +38,11 @@ typedef struct YGSize {float height;isDefined
@@ -108,12 +113,26 @@ WIN_EXPORT void YGNodeCopyStyle(const YGNodeRef dstNode, const YGNodeRef srcNodeWIN_EXPORT void YGNodeStyleSet##name(const YGNodeRef node, const type paramName); \WIN_EXPORT type YGNodeStyleGet##name(const YGNodeRef node);#define YG_NODE_STYLE_PROPERTY_UNIT(type, name, paramName) \##name##( -> ##name(
@@ -38,6 +38,11 @@ typedef struct YGSize {float height;great idea!
@woehrl01 updated the pull request - view changes
There are some merge conflicts according to github, otherwise looks good. Will try to import internally tomorrow.
@woehrl01 updated the pull request - view changes
did a merge with the upstream and modified the c# wrapper. All c# tests are green for me.
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
Seems like the benchmarks, csharp, and java tests are still failing. I can take a look tomorrow
@woehrl01 updated the pull request - view changes
@woehrl01 updated the pull request - view changes
It would be great if you could take a look at java. It's ages for me the last time I did some JNI.
The java version will take a bit more work to update. I'm not super familiar with JNI either tbh. But i'll have a look this week.
Did some performance improvements (~20%) on my pc:
@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@woehrl01 While implementing the JNI bindings I thought of a slightly different way to implement this. Wanted to get your thoughts on it.
Instead of
YGValuewe store theYGUnitand theisDefinedflag as part of the numeric value itself. We could reserve a couple bits in the unit's value for these flags. This would require switching fromfloatvalues touint64types, the first 32 bits storing the meta data and the second 32 bits storing the floating point value.This would allow for a couple things.
YGNodeStyleSetWidth(node, 10)would still set the width of a node to the pixel value of 10.YGValueconstructor when using percentage (which should be much more seldom than pixel values).YGNodeStyleSetWidth(node, YGPercent(10))would set the width of the node to 10%.The major downside is that we would no longer allow setting a floating point value directly as the parameter is now a integer,
YGNodeStyleSetWidth(node, 10.5)would be a compile time error. We might be OK with this though as setting half pixels on styles is generally discouraged anyways. The thing to keep in mind here is platforms not using Yoga with pixels but points instead (iOS). This would require them to transform pixel value to points before passing values to Yoga (but we control the bindings here so the end user should not need to care). @dshahidehpour What do you think about this?This idea came from the implementing the java bindings as there we need to do this either way to avoid allocations and passing larger objects over JNI which can be much more expensive than primitive types such as floats and integers.
@emilsjolander
Sounds good! I have an extended idea on top of it.
if you call
YGNodeStyleSetWidth(node, 10)you will set an u_int64 where the upper bits are fully 0. Which means we provide an int in the lower. If we call it withYGNodeStyleSetWidth(node, YGPx(10.5))we store the lower as a float and add an additional flag in the upper bits to detect this. So we can still set floating values via the api, we just need to be explicitly useYGPxthen.Idea 1: Regarding points and pixel. Maybe we should rename it and use Points instead of Pixel all over.
Idea 2: We extend the Api with an additonal Points value factor (like setting the logger). So we can also do the rounding with higher density + an additional YGUnitPoints where we calculate
unit.value * densityFactorwhich addionally provides subpixel rounding.I think this is a huge piece of functionality to throw away. As you correctly stated in your comment, iOS deals with Points, not Pixels. If someone gave us a design where they wanted to something to be 21 pixels wide (very reasonable), it would no longer be possible to use Yoga on iOS because this is how you would define value so of 21 pixels:
So, I'm very much against that.
I like this API, but, I think we would have to rename
YGPxto something else. I don't think that Yoga should know (or care) what the units of a coordinate system are. It should only care that you are dealing with absolute values or ratio values (percents). For all we care, someone could be using Yoga to calculate the layout of their new backyard and all the measurements are in meters.cc: @emilsjolander
We are going to need to introduce some notion of Points into Yoga at some point. Currently pixel rounding (behind experiment) is kinda broken on iOS as it does not take the point scale factor into account.
So we could end up with an API similar to this where pixel values have to be integers but both points and percentage values can be floating point. The point scale factor would also be taken into account when performing pixel rounding.
I agree that the API for iOS developers still has to be nice. But I think this is where
YogaKitplays a vital role.YogaKitallows us to hide some of this detail behind the scenes and always just default to point values and setting the scale factor automatically.@dshahidehpour how does that sound? I'm not by any means certain about this.
@emilsjolander do you want to replace the struct inside the c, too? Could reduce the memory footprint of new nodes as currently we do not perfectly memory align due to the bool flag.
@woehrl01 yeah, as I see it
YGValuewould just be an alias for auint64_t. But please don't start making any changes yet as we aren't certain about this yet 👍 Don't want you to waste any time@emilsjolander no worries 😄 . But how about negative values?
YGNodeStyleSetPosition(node, YGEdgeTop, -10)would possibly throw some warnings.haha yeah, so perhaps
int64_tand notuint64_t:pRegardless of whether or not the experiment is running, we always round the values in
YogaKitto make sure they correct.YGRoundPixelValuedoes take the devices scale into account.That sounds kinda complex to me. Wouldn't it make more sense to remove units from this library completely and generalize it? I'd rather leave the burden of points, pixels to
YogaKitand keep the C-library agnostic of different units.Other platforms also have some notion of 'Points'. Moving this into Yoga would allow us to keep the rounding logic in one place.
I don't disagree. I would prefer if we could have an api more like this. Where the default scale factor is 1, making pixels the default but allowing users to specify another scale factor to use the value as a point (would not be part of this PR but a possible future addition).
However this is not possible as we can't store bit flags on a floating point value. So the alternative then is to have any api where percentage values are specified via a different function call, which I would be fine with as well.
--
I think it boils down to having on of these two APIs. (I included the scale factor api for show but that would be a different pull request ofc).
1
2
--
I think I prefer 2 as that would allow all existing code to function as it currently does and make it clear when using percentage values. A previous version of this PR had a very similar API
YGNodeStyleSetWidthWithUnitbut in that case there would be two ways of setting pixel values which I would prefer not to have. The argument can be made that exposing aUnitsetter would allow the addition of more units in the future without adding to the API, however I don't see us adding other units in the near future (I could of course be very wrong on this point).What option do you both prefer? Or would you prefer a third alternative?
I would also prefer #2!
I would still prefer to provide the unit with one function call. I think this would make the interop call much simpler, as we don't have to call different functions.
I would not like to have a
.width+.widthPercentmethod inside my code or do some magic trickery behind the setters.So if want to have an "no unit" setter and no "struct" setter, I would make a
Not sure how you want to return the value in the getter, if there is percentage set?
you would need to guess if you need to call
YGNodeStyleGetWidthPercent()orYGNodeStyleGetWidth()Calling set with a different type, where get returns another is also not very ideal.
So I would prefere nummero 1 😄
Even with option 2 the return value of the getters would be of the current
YGValuetype, otherwise as you say you would have to guess the correct getter.I don't think the language bindings have to match the C API exactly. The language bindings should use an API with suites that language best. in java this will probably be
setWidthPercent()but as the .NET bindings use properties and not methods aYGValuetype abstraction in c# would probably make most sense.I fully agree, thats the reason I would like to make the C API as generic as possible. Which leads me to your first suggestion. I also think that in Java
setWidthPercent()could be a great name.I think leaving
YGNodeStyleSetWidth(node, 10.0)working as it does today is a good idea becauseSo I think the only open question right now is wether to have
YGNodeStyleSetWidthPercent(YGNode, float)YGNodeStyleSetWidthWithUnit(YGNode, YGUnit, float)In 2,
YGNodeStyleSetWidth(YGNode, float)would just be a shorthand forYGNodeStyleSetWidthWithUnit(YGNode, YGUnitPixel, float).I prefer
YGNodeStyleSetWidthPercent(YGNode, float)as it is a more clear name and the main benefit of the more genericYGNodeStyleSetWidthWithUnit(YGNode, YGUnitPixel, float)is the ability to add more units later which I don't expect we will and if we do then we can tackle it then. I don't want to create a very generic solution where one is not needed.As I earlier the return value of the getters will still need to be of type
YGValue.I totally understand the thoughts behind that API. The only thing the bugs me, is that the getter and setter have different types or at least there isn't any matching method pair.
Having this kind of api, is not very self explaining, imo:
@emilsjolander @dshahidehpour By the way: if you both think this is the prefered way, I'm totally fine doing this. I just want to play some kind of devil's advocate and challenge the decision.
@emilsjolander some additional ideas / thoughts:
What I like about suggestion 2:
It's hard to abuse it. We don't have to check if the provided values, are a matching UnitType as we provide the unit via the function name.
We could hide the unit enum completely and add an additonal "UndefinedUnit", so we don't have to do some magic bit fiddling in the upper part for an "isDefined" bit.
Not sure if it's a good idea to do a typedef for
YGValuebecause of the implicit cast possibilities. I'd prefere a struct likeYGSize.Agreed, it is not perfect. I think it is the best option now though. Let's make sure to document it!
No reason to do any bit fiddling with this api. Keep the
YGValuestruct andYGUnitas is in the current state.what's your opinion about the idea of putting Definied as a "unit"? As an undefined YGPx + undefined YGPercent doesn't really make sense, imo.
so we can at least reduce the byte for the bool flag and won't leak any implementation detail into the YGValue return struct.
I'm fine with that 👍
@emilsjolander Cool 👍 ! If there aren't any other opinions, I'd like to summarize our decision before I "get my hands dirty": 👏
SetWidth(node, float)+SetWidthPercent(node, float)YGUnitUndefinedQuestions/Brainstorm:
How about the naming of the unit for pixel? As @dshahidehpour pointed out, some users won't care, should this be named differently? Maybe
YGUnitAbsolute,YGUnitRelativeinstead of pixel and percent?Summary looks good. Again, you can leave the JNI stuff to me if you want. Regarding
YGUnitPixelI think it is fine as is. Most usages are pixels or some form of pixel abstraction such as points. Trying to generalize to any possible unit just makes it more confusing imho.That would be great, thanks!
agree 👍
Tests still include a bunch of statements such as
YGNodeStyleSetWidth(root, YGPx(100))which should beYGNodeStyleSetWidth(root, 100)according to the previous discussions.@@ -8,6 +8,12 @@# Visual studio code.vscode*.pdbplease mirror these changes in .hgignore
@@ -17,6 +17,11 @@ ENUMS = {'LTR',indentation
Disregard. I see you still have commits you are pushing 👍
@@ -17,6 +17,11 @@ ENUMS = {'LTR','RTL','Unit' is still wrongly indented compared to other enums
@@ -17,6 +17,11 @@ ENUMS = {'LTR','RTL',😖 sorry, will do (all my editors are configured to use tabs instead of spaces)
benchmarks, C tests, and c# tests are failing. Once those are passing i'll import and fix the java and obj-c bits.
As I don't have clang, could you please lead me to the right direction how to fix this:
edit: got it
@woehrl01 In addition to the float extension methods could we add an implicit conversion operator from float -> pixel YGValue? https://msdn.microsoft.com/en-us/library/85w54y0a.aspx
should be already there (YogaValue.cs line: 69)
Then we should be able to remove the
.Px()from the tests right?true, will do. gimme a minute. 🕐
I'm happy to start importing this now. Just want to make sure you don't have anything else in the process of being committed?
No additional changes 👍 if you have no further requests 😉
@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This may be a dumb question, but how and when can one start to utilize this feature in RN? I'm not familiar with how yoga is versioned into RN. Amazing and very useful work here! Thank you!
@srhise once this is merged RN will be able to expose it by adding percent to their API. I have no ETA to give on this.
Pull request closed