-
Notifications
You must be signed in to change notification settings - Fork 25
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
Enhance Color #71
Enhance Color #71
Conversation
90a1c20
to
334436e
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.
There are at least two issues blocking this merge; see comments inline.
@@ -81,7 +81,7 @@ pub fn instantiate_expression_table<R: 'static + RenderContext>() -> HashMap<usi | |||
{% endif %} | |||
|
|||
TypesCoproduct::{{ expression_spec.pascalized_return_type }}( | |||
{{ expression_spec.output_statement }} | |||
{{ expression_spec.output_statement }}.into() |
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 believe this is problematic. I set up repro here:
https://github.com/pax-lang/pax/compare/6df4802c6b32^...78fb7765dcb2#diff-22275c553b4a7bfe13d35b04f59015dba9373818c30049484349acdb2a4627f4L3
I started with your branch, rebased off of origin/master
so we have Warfa's latest work on /examples
, then modified the fireworks
example to be able to reproduce the error I'm seeing related to this .into
.
You should be able to reproduce by pulling the branch oa/pax_qualia
(from this repo) and running cargo run --examples fireworks
. The error:
error[E0277]: the trait bound `isize: From<i32>` is not satisfied
--> /Users/zack/code/pax/examples/src/fireworks/.pax/pkg/pax-cartridge/src/lib.rs:110:19
|
110 | 0..60.into()
| ^^^^ the trait `From<i32>` is not implemented for `isize`
This is on the range expression for i in 0..60
— the addition of the .into
breaks that range literal. If we need to add an additional .into
for Color, can it be higher in the codegen than this terminal statement? Because of the constraints around the graph of types we can into
, each new into
comes with a cost, so is it possible to drop this one entirely?
} else if color_format.starts_with("rgba(") && comma_count == 3 { | ||
format!("Color::rgba({r},{g},{b},{a}).into()") | ||
} else if color_format.starts_with("hlc(") && comma_count == 2 { | ||
format!("Color::hlc({r},{g},{b}).into()") |
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'm not sure this code path is being hit. When I try fill={hlc((ticks + i * 360.0 * 3)%, 50%, 50%)}
in the Fireworks example, I get an error:
168 | ... hlc((Size::Percent((ticks +((i *(Numeric::from(360.0 )).into())*(Numer...
| ^^^ not found in this scope
My best guess from a quick look is that we might be short-circuiting this on line 211 of this file: https://github.com/pax-lang/pax/pull/71/files#diff-3bfd18387cf178bec2da888bfa3aadadd3de4522ed242e2b303877b0a3dcb855R211 , but the issue might be something else entirely.
This pr does the following
42%
Rectangle fill = rgba(100.0%,0.0%,0.0%,100.0%
Rectangle fill = #123456
Color
toFill