Add fields and methods to several primitives #790

PgBiel · 2023-04-13T20:34:11Z

Closes #753.

This PR adds the following fields and methods to primitive types (please feel free to suggest more):

length: fields em (get em part), abs (get pt part); methods cm (converts pt to a floating point cm), mm (same but to mm), inches (same but to inches).
relative length: fields ratio (get ratio part) and length (get absolute/length part)
color: methods kind (the constructor used: rgba() | cmyk() | luma()), rgba (converts to array of rgba values), hex (converts any color to a RGBA hex string prefixed by #), cmyk (converts a luma or cmyk color to an array of cmyk ratio values, or throws an error if it's an rgba color as there is no such conversion), luma (returns the value of a luma color, or throws an error if it's an rgba or cmyk color)
stroke: fields paint, thickness, cap, join, dash, miter-limit
direction: now has methods .axis() ("horizontal" for rtl and ltr, or "vertical" for btt and ttb), .start() (left for ltr, right for rtl, top for ttb, bottom for btt), .end() (same idea, but inverted, e.g. right for ltr), and .inv() (rtl for ltr, ltr for rtl, etc.)
alignment: now has methods .axis() ("horizontal" for left, right, start, end, center; "vertical" for top, bottom, horizon) and .inv() (left<->right; top<->bottom; start<->end; center<->center; horizon<->horizon)
2d alignment: now has fields .x and .y (they return the x and y components of the 2d alignment, respectively; e.g., (left + top).x == left and (left + top).y == top), and method .inv() (inverts both components; e.g. (right + bottom).inv() == left + top, while (center + horizon).inv() == center + horizon).
angle: now has the methods rad and deg (return a float with the conversion of this angle to radians and degrees respectively).

Tests were also added. (Still missing docs for all this stuff, though.)

I'm marking this PR as draft so I can have time to add fields for the new stroke attributes (which were pushed recently - this branch isn't rebased yet); I just wanted to show what I have so far so we can discuss it properly.

Any thoughts? Any other type I should add fields for? Also, I wonder if it would be appropriate if some of these fields were converted to methods (if so, which? Maybe cm, mm, inches, and the color conversion ones?).

PgBiel · 2023-04-14T16:43:23Z

By the way, I wonder in which missing_fields functions should #[track_caller] be added? I added it to all so far, but just wanted to be sure regarding which criteria I should use here

laurmaedje · 2023-04-20T09:11:56Z

By the way, I wonder in which missing_fields functions should #[track_caller] be added?

There are a few stupid track_callers in the code base for some reason. We only need it if the handler panics, not if it returns a string.

PgBiel · 2023-04-20T09:35:51Z

There are a few stupid track_callers in the code base for some reason. We only need it if the handler panics, not if it returns a string.

Ohhh alright, thanks for the insight. I might be updating this PR soon with some changes, and I'll remove the unneeded track_callers from my code lol.

laurmaedje

one thing I'm overall also not sure about is fields vs methods. I feel like most of these should be methods. the thing I can see being fields the most is color and thickness. maybe the policy could be which are conversion vs. which are just accessors. the color field has the additional problem that it won't necessarily be a color anymore once we have gradients.

laurmaedje · 2023-04-23T12:40:12Z

src/geom/length.rs

+
+    /// Get a field from this length.
+    pub fn at(&self, field: &str) -> StrResult<Value> {
+        let round_four_digits = |n| ((n as f64) * 1e4).round() / 1e4;


This was because floating point imprecision was causing issues - 3cm wouldn't be equal to (3cm).cm because the latter would return something like 3.000000004. However, I think it would be better if we turned .cm and other conversions into methods where you can specify the precision.

laurmaedje · 2023-04-23T12:40:49Z

src/eval/value.rs

+            d @ Self::Dyn(dynamic) => {
+                if let Some(stroke) = dynamic.downcast::<PartialStroke>() {
+                    stroke.at(field)
+                } else {
+                    Err(eco_format!("cannot access fields on type {}", d.type_name()))
+                }
+            }


Suggested change

d @ Self::Dyn(dynamic) => {

if let Some(stroke) = dynamic.downcast::<PartialStroke>() {

stroke.at(field)

} else {

Err(eco_format!("cannot access fields on type {}", d.type_name()))

}

}

Self::Dyn(dynamic) => {

if let Some(stroke) = dynamic.downcast::<PartialStroke>() {

stroke.at(field)

} else {

Err(eco_format!("cannot access fields on type {}", dynamic.type_name()))

}

}

laurmaedje · 2023-04-23T12:41:01Z

src/geom/length.rs

+#[cold]
+#[track_caller]


Suggested change

#[cold]

#[track_caller]

laurmaedje · 2023-04-23T12:41:16Z

src/geom/paint.rs

@@ -1,3 +1,5 @@
+use crate::eval::{Array, Str};


crate imports should be below, together with the super

laurmaedje · 2023-04-23T12:42:01Z

src/geom/paint.rs

+            "cmyk" => match self {
+                Self::Cmyk(cmyk) => cmyk.to_array().into(),
+                Self::Luma(luma) => luma.to_cmyk().to_array().into(),
+                _ => Value::None, // no rgba -> cmyk conversion


not sure about just returning none here to be honest

Yeah, I was worried because you wouldn't be able to prevent an error if the color type were unexpected (note that I'm basically trying to avoid many usages of repr here)... Perhaps we can add a color-type method?

^As an update to this specifically: I moved this to a method to-cmyk(), which now errors on the previously "None" case. In order to be able to prepare for the error, I added the .kind ~~field~~ method to colors ~~(though I might also move that to a method, but not 100% sure yet).~~ (EDIT: .kind() is now a method.)

PgBiel · 2023-04-23T17:12:17Z

one thing I'm overall also not sure about is fields vs methods. I feel like most of these should be methods. the thing I can see being fields the most is color and thickness.

Yes, for sure! Thanks for the feedback. Indeed, I actually intend to change most of these to methods; I mostly wanted to see how far I could get with those attributes and stuff
I'll try to work on this PR a bit today, but I'll probably only be truly finished in several days (due to upcoming exams haha)

maybe the policy could be which are conversion vs. which are just accessors.

Makes sense to me.

the color field has the additional problem that it won't necessarily be a color anymore once we have gradients.

Well, I guess the implementation here will depend on how we handle gradients. Perhaps have some sort of is-solid() method? (Or maybe use the color-type idea I mention above?)

PgBiel · 2023-05-21T06:17:46Z

Alright, so, updated the branch to latest main (adding fields for the stroke things). I'm still planning to add a bit more stuff (and also moving several fields to methods) before marking this PR as ready for review.

In the last few commits, it's worth saying that I made a few sorta significant changes. In particular, I created eval/fields.rs, akin to eval/methods.rs, to organize functions related to fields, including a fields_on function. I needed to make such a move in order to implement field autocomplete on ide/, but there might be a different/better way to do this (feel free to suggest changes here).

To be honest, I'd normally prefer some sort of trait system to be able to have value types implement their fields by themselves, though I acknowledge this would be hard to do (perhaps in a future PR or something, if it ever happens - maybe if the 'methods-as-fields' idea goes forward). So this should work.

PgBiel · 2023-05-21T07:05:17Z

Regarding colors, I made the following changes: added the kind field to tell different color types apart (rgba vs cmyk vs luma) and the values field to return the color's values (i.e., for rgba, returns (R, G, B, A) as ints; for cmyk, returns (C, M, Y, K) as ratios; for luma, I made it return a unit array (luma_value,) for type consistency (I can change this if you want).

Though, I feel like returning the CMYK ratios is hacky, as obtaining the original ratios given is not 100% possible. I think we should just accept both ratios and ints as valid inputs to cmyk(), and then always display ints in repr() and .values, which would fix #787. (Thoughts? This is very trivial to do, but could be made in a separate PR if you prefer.)

Finally, we should probably decide on some documentation format for type fields, preferably similar to their methods. Feel free to send suggestions; I will try to take a look on this later (not too worried about this right now).

laurmaedje · 2023-05-21T08:39:12Z

Great to see work on this resuming. If there are many fields, an extra module certainly makes sense although I share your desire for having this more trait based in the future. One big question I have: Didn't we want to make most of these methods?

PgBiel · 2023-05-21T09:52:12Z

One big question I have: Didn't we want to make most of these methods?

Yes! Slowly working on that, haha (sorry, forgot to mention this explicitly in my update). Currently I have only done this for colors so far: .kind and .values are fields as they are pretty basic and don't require much logic, while all the conversion things - hex, cmyk, rgba - were moved to methods. (.luma became unnecessary with .values)

Though, I'm guessing that .kind could also be moved to a method (EDIT: done!), as I guess it's sort of generated on the fly. But .values IMHO makes sense as a field for me, as it might even make sense to perhaps make it assignable 👀 .

Either way, I'll be working on moving other types' stuff to methods next ;)

PgBiel · 2023-05-23T04:11:35Z

Alright! Today's update:

Rebased to latest main (after all the wonderful bugfixes);
Changed precision: to digits: in .cm(), .mm(), .inches() for consistency with calc.round()
- Note that I kept this argument because precision sucks here with typical floats, so 3.345cm == (3.345cm).cm() wouldn't normally hold, for example (due to some digit at the very end). So I defaulted precision to 10 digits (such that the equality holds) while allowing for 15 (with digits: 15), but, ideally, we'd use some sort of BigDecimal library. We can also just return the float straight away and force the user to calc.round if that's preferrable.
New field/method implementations:
- direction: now has methods .axis() ("horizontal" for rtl and ltr, or "vertical" for btt and ttb), .start() (left for ltr, right for rtl, top for ttb, bottom for btt), .end() (same idea, but inverted, e.g. right for ltr), and .inverse() (rtl for ltr, ltr for rtl, etc.)
  - Note that I omitted the internal is_positive method here as I didn't find much use for it for users (/could be confusing), but feel free to tell me if I should expose it here as well.
- alignment: now has methods .axis() ("horizontal" for left, right, start, end, center; "vertical" for top, bottom, horizon) and .inverse() (left<->right; top<->bottom; start<->end; center<->center; horizon<->horizon)
  - Regarding .axis() I made start and end return horizontal as that's how it's done internally, but we could wish to error instead as start and end could, I guess, have a vertical axis (if typst ever adds support for this). This kind of future-proofing could be excessive though, which is why I am mentioning this here just for awareness (it's likely fine to keep it as horizontal for now, even if for consistency with how 2d alignment works).
  - Regarding .inverse(): I considered that, while start and end can technically be arbitrary, in practice, we shouldn't ever expect them to not be the inverse of each other. (Why would they have the same value, or different axes?)
- 2d alignment: now has fields .horizontal and .vertical (they return the x and y components of the 2d alignment, respectively; e.g., (left + top).horizontal == left and (left + top).vertical == top), and method .inverse() (inverts both components; e.g. (right + bottom).inverse() == left + top, while (center + horizon).inverse() == center + horizon).
  - Feel free to tell me if you'd prefer x and y as the field names (instead of horizontal and vertical), for example.

And that's mostly all for today (besides other minor changes). (Just sharing this to make it easier to stay in touch with this PR's progress.)

laurmaedje · 2023-05-23T08:39:37Z

I think users can round themselves, the .cm() method should just perform the conversion
inverse() sounds a bit like an in-place operation. Perhaps just inv()? That would also be consistent with some symbol modifiers.
You can safely assume that start and end are horizontal for now, that's a general assumption.
start and end are always the inverse of each other
I prefer .x and .y over .horizontal and .vertical

PgBiel · 2023-05-23T08:53:56Z

Thanks for the valuable input!! Will work on your suggestions later today. 👍

PgBiel · 2023-07-08T23:17:01Z

@laurmaedje the PR seems to be mostly complete now. What's truly missing is documentation, and, for that, there are two blockers (I've added what I could so far though):

stroke, direction, alignment and 2d alignment don't seem to be in types.md. Should we add sections for them (to accommodate their fields / methods)?
What should be the format to document fields and their respective types for a particular type? I was thinking of something like

# Stroke
(...)

## Fields
### paint
The color that a line with this stroke would use.

- type: color

## Methods
(... none in this case so pretend this isn't here ...)

PgBiel

Since the code is pretty much already ready for review though, I'll leave some comments on parts I think would be of most interest during a review. Of course, you're free to leave other comments in other parts (and also to change documentation / comments at will).

Feel free to suggest any missing tests as well.

crates/typst/src/geom/axes.rs

PgBiel · 2023-07-08T23:24:21Z

crates/typst/src/geom/stroke.rs

@@ -322,6 +322,7 @@ impl Resolve for DashPattern {
 // https://tex.stackexchange.com/questions/45275/tikz-get-values-for-predefined-dash-patterns
 cast! {
    DashPattern,
+    self => dict! { "array" => self.array, "phase" => self.phase }.into_value(),


I made dash pattern (returned via .dash field of a stroke) be converted to a dict with the same structure as the DashPattern struct. I thought of maybe comparing with the values for "solid", "dotted" etc. to return those strings if applicable, but I settled with this in the end (it's a simpler solution, at least, and encompasses all cases). Feel free to suggest any changes.

Note that we can't directly test this conversion yet, as you can't mutate fields for now (this would be handled by a follow-up PR), and the stroke type only appears directly for the user through expressions like 2pt + blue - anything more complicated (with a different dash pattern than the default none) would be expressed by a dict, whose values the user can retrieve directly.

You can observe it by doing something like #rect(stroke: (thickness: 1pt, dash: "solid")).stroke. Looking at the current output from that, some Debug impl is suboptimal (it contains Some). I'm not super happy with the stroke setup in any case. A stroke constructor function instead of the dictionary would maybe make sense.

Not returning strings like "solid" is the right call. The structure should be uniform and "canoncalized" to to speak. Just like grid(columns: 2).columns == (auto, auto).

You can observe it by doing something like #rect(stroke: (thickness: 1pt, dash: "solid")).stroke.

Oh, didn't think of that. I'll add some tests using this. Thanks!

Looking at the current output from that, some Debug impl is suboptimal (it contains Some).

The following lines seem to be the case:

Debugging Option<DashPattern> directly:

typst/crates/typst/src/geom/stroke.rs

Line 162 in 507efc3

write!(f, "{}dash: {:?}", sep, dash)?;

Deriving Debug for DashLength:

typst/crates/typst/src/geom/stroke.rs

Lines 351 to 352 in 507efc3

#[derive(Debug, Clone, Eq, PartialEq, Hash)]

pub enum DashLength<T = Length> {

I'll take a look at those. Perhaps Typst should get its own Debug-like (e.g. Repr) trait at some point though, to avoid this kind of issue (since we can't re-implement Debug for Option, a built-in type). But that's just a thought, and perhaps a very wrong one :p

I'm not super happy with the stroke setup in any case. A stroke constructor function instead of the dictionary would maybe make sense.

I agree; this could be implemented in the type rework.

I agree on the Repr trait, I've actually implemented it at one point. I don't know what stopped from merging it anymore.

Ok, fixed debugging, and added some more tests too. 👍

crates/typst/src/geom/stroke.rs

crates/typst/src/ide/complete.rs

crates/typst/src/eval/library.rs

laurmaedje · 2023-07-10T10:24:04Z

crates/typst/src/eval/fields.rs

+    let not_supported = || Err(no_fields(name));
+    let missing = || Err(missing_field(name, field));
+
+    // Special cases, such as module and dict, are handled by Value itself


why not handle everything here?

I did that initially, but it felt a bit inconsistent with the fact that fields_on only returns a list of static fields (not dynamic like dict's fields), much like methods_on and methods.rs don't handle methods in a module / function scope. I can move things back here if you prefer, though.

Leave it as-is then.

crates/typst/src/eval/methods.rs

laurmaedje · 2023-07-10T10:30:13Z

crates/typst/src/eval/methods.rs

@@ -152,6 +179,19 @@ pub fn call(
            _ => return missing(),
        },

+        Value::Length(length) => match method {
+            "cm" => length.abs.to_cm().into_value(),


I think pt should be here to and then we drop the abs field. There is no reason why pt should be special.

It could be confusing that this just ignores the em part, but not much we can do about it (without future lazy magic get rules). But probably em should then also be a method here and we drop all fields?

We could also keep the abs field, add pt() here and have these functions throw if em is not 0. then you can do length.abs.cm(), but 2em.cm() fails instead of giving 0.0.

We could also keep the abs field, add pt() here and have these functions throw if em is not 0. then you can do length.abs.cm(), but 2em.cm() fails instead of giving 0.0.

This sounds more appropriate to me. Perhaps we should add this as a hint as well

Done. Make sure to check the error/hint messages and the docs for any changes you might deem necessary.

crates/typst/src/geom/align.rs

crates/typst/src/geom/color.rs

laurmaedje · 2023-07-10T10:38:58Z

Currently stroke, direction, alignment, and alignment 2d link to some function they are often used with. An extra type page would be okay I guess, I didn't do it that way because the Types section would grow very large and not all types are equally fundamental. There's also regex for instance.

Adding a new section to types.md seems like a lot of investment (for you in the docs crate and for me in the actual docs generator) into a file I'd like to see removed. Maybe it's best to just document the fields in the opening text for the type as done currently for things that have fields?

With my unmerged type rework, the Types section was completely removed, and there were pages for the types in the appropriate categories which makes more sense in my opinion. I didn't finish work on it because other important things came up, but it was also not completely clear how the changes would integrate with other ideas for scripting (e.g. elements as types, lazyness + get rules, etc). Maybe I should still finish it as it would perhaps give a better starting point for future work than we currently have. (And remove the weird distinction between scoped functions and methods.)

PgBiel · 2023-07-10T21:04:44Z

Currently stroke, direction, alignment, and alignment 2d link to some function they are often used with. An extra type page would be okay I guess, I didn't do it that way because the Types section would grow very large and not all types are equally fundamental. There's also regex for instance.

Hmm. Perhaps I can just add this info in a summarized manner then in the docs for each of those "minor types". E.g. "the method .inv() returns the inverse alignment ...". Ideally, we'd have a separate page for each one; however, your proposal of having type pages in their respective categories could probably be more appropriate for this.

Adding a new section to types.md seems like a lot of investment (for you in the docs crate and for me in the actual docs generator) into a file I'd like to see removed. Maybe it's best to just document the fields in the opening text for the type as done currently for things that have fields?

Sure, sounds good to me.

With my unmerged type rework, the Types section was completely removed, and there were pages for the types in the appropriate categories which makes more sense in my opinion. I didn't finish work on it because other important things came up, but it was also not completely clear how the changes would integrate with other ideas for scripting (e.g. elements as types, lazyness + get rules, etc). Maybe I should still finish it as it would perhaps give a better starting point for future work than we currently have. (And remove the weird distinction between scoped functions and methods.)

I agree it probably makes sense. I think there should still be a way to list the currently available scripting types, however, in some sort of "index", as this has been pretty useful to me. (Perhaps this idea could be extended to elements as well, in a separate page.)

PgBiel · 2023-07-10T21:53:46Z

Alright, I believe I've added docs for everything now. All that's left is mostly reviewing stuff, adapting docs as needed, etc. I'll be undrafting the PR then, since the main goals were already achieved; feel free to re-draft if you feel that's appropriate, though.

laurmaedje · 2023-07-11T14:11:34Z

Thank you! :)

This was referenced Apr 15, 2023

API to distinguish between pt-length and em-length? #819

Closed

repr inconsistent parse of #841

Closed

laurmaedje reviewed Apr 23, 2023

View reviewed changes

reknih force-pushed the main branch 4 times, most recently from 102fa05 to b79121d Compare April 23, 2023 17:35

PgBiel mentioned this pull request May 17, 2023

error: cannot compare length and length #1231

Closed

1 task

PgBiel force-pushed the parts-of-primitives branch from 2ac6fa7 to 17ba178 Compare May 21, 2023 04:00

PgBiel changed the title ~~Add fields to several primitives~~ Add fields and methods to several primitives May 22, 2023

PgBiel force-pushed the parts-of-primitives branch from dc5b1fb to 2c74112 Compare May 23, 2023 01:53

PgBiel mentioned this pull request May 24, 2023

Allow ints for CMYK to ensure consistency #1307

Closed

PgBiel added 7 commits May 25, 2023 00:44

add fields for rel&stroke parts

8c4bdb0

add color fields

40437d3

fix paint fields + add hex

b2c3adb

have cmyk ratio rounding match the repr

36dca62

add some fields tests

af274b7

add length fields

bd1b496

paint.rs formatting

c46b030

remove redundant docs text on returning

981485e

PgBiel commented Jul 8, 2023

View reviewed changes

laurmaedje reviewed Jul 10, 2023

View reviewed changes

PgBiel added 11 commits July 10, 2023 15:18

return Err => bail

548ae9f

remove redundant text dir comment

cc66916

remove excessive newline in geom/color.rs

7567bd0

value completion for type fields

fecd856

fix tests' error messages

23c558f

fix stroke debug

f5e11f7

add tests for complex stroke fields

805d890

error on length unit conversion for 'em'

219bb36

add length.pt()

a556e2b

update length docs

700e52f

fix length docs method reference

d434448

PgBiel added 4 commits July 10, 2023 18:15

add length field docs

d231d16

improve length field docs wording

51c9005

add relative length field docs

89208c0

add field/new method docs for 'minor types'

a8e065e

PgBiel marked this pull request as ready for review July 10, 2023 21:53

laurmaedje removed the waiting-on-author Pull request waits on author label Jul 11, 2023

laurmaedje merged commit 9b1a2b4 into typst:main Jul 11, 2023
4 checks passed

PgBiel deleted the parts-of-primitives branch July 11, 2023 21:11

PgBiel mentioned this pull request Jul 17, 2023

Update field mutation error message #1742

Merged

Gewi413 mentioned this pull request Jul 22, 2023

Adding little arrows on axis Pegacraft/typst-plotting#12

Merged

dixslyf mentioned this pull request Dec 3, 2023

Inconsistent rounding of the repr of length, relative, fraction, ratio and angle values #2836

Open

1 task

	#[derive(Debug, Clone, Eq, PartialEq, Hash)]
	pub enum DashLength<T = Length> {

Add fields and methods to several primitives #790

Add fields and methods to several primitives #790

Conversation

PgBiel commented Apr 13, 2023 • edited Loading

PgBiel commented Apr 14, 2023

laurmaedje commented Apr 20, 2023

PgBiel commented Apr 20, 2023

laurmaedje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgBiel May 21, 2023 • edited Loading

Choose a reason for hiding this comment

PgBiel commented Apr 23, 2023 • edited Loading

PgBiel commented May 21, 2023 • edited Loading

PgBiel commented May 21, 2023

laurmaedje commented May 21, 2023

PgBiel commented May 21, 2023 • edited Loading

PgBiel commented May 23, 2023

laurmaedje commented May 23, 2023 • edited Loading

PgBiel commented May 23, 2023

PgBiel commented Jul 8, 2023

PgBiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgBiel Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurmaedje commented Jul 10, 2023

PgBiel commented Jul 10, 2023

PgBiel commented Jul 10, 2023

laurmaedje commented Jul 11, 2023

PgBiel commented Apr 13, 2023 •

edited

Loading

PgBiel May 21, 2023 •

edited

Loading

PgBiel commented Apr 23, 2023 •

edited

Loading

PgBiel commented May 21, 2023 •

edited

Loading

PgBiel commented May 21, 2023 •

edited

Loading

laurmaedje commented May 23, 2023 •

edited

Loading

PgBiel Jul 10, 2023 •

edited

Loading