Shaders: more graster-nodes no-std fixups
#3090
Conversation
| GradientStops, | ||
| )] | ||
| mut image: T, | ||
| #[default(Color::BLACK)] tint: Table<Color>, |
There was a problem hiding this comment.
Please verify that changing Table<Color> -> Color indeed changes absolutely nothing, since with the table you're just using the first entry of the table anyway. Affects both this black_and_white node and the color_overlay node.
There was a problem hiding this comment.
This change shouldn't occur, we do need it to take a Table since that is the only data type relating to colors that is processed within the node graph.
There was a problem hiding this comment.
I now replaced Table<Color> with Option<Color> (instead of just Color) and added an into_node!(from: Table<Color>, to: Option<Color>),, since we already have a impl From<Table<Color>> for Option<Color>. So there's no need to adjust anything in the preprocessing!
However, both Color and Option<Color> makes the tint property disappear in the properties panel.
There was a problem hiding this comment.
I tried adding Option<Color> to TaggedValue to the UI to show, but I couldn't get it to work. Would appreciate it if someone with more editor experience took a look, so I can focus on other important things.
There was a problem hiding this comment.
In PR #3048, Keavon removed Color and Option<Color> from the properties system and also from the TaggedValue (used to serialize properties). This seems to me to be a bad idea. I wonder what your opinions are on this matter?
There was a problem hiding this comment.
The into nodes should now be added automatically if needed. The gpu node declares it's input as Color or Option<Color> but you can still feed a Table<Color> as input.
There was a problem hiding this comment.
I agree. Even on the CPU side, there is no need for nodes that work only on a single colour to accept a table.
There was a problem hiding this comment.
And this is approved by Keavon (at least for the GPU case). @0HyperCube if you want you can make a pr to just revert the removal of Color and Option<Color> from the node_properties
There was a problem hiding this comment.
I agree. Even on the CPU side, there is no need for nodes that work only on a single colour to accept a table.
And I think there is an argument to be made that now that we have automatic into node insertion we should revert this for nodes which only ever operate on a single color, but I assume @Keavon will have opinions on that
There was a problem hiding this comment.
Let's please do Option<Color> only, not Color, since this is somewhat temporary as discussed in our Discord conversation.
Firestar99
force-pushed
the
shaders-graster-prep3
branch
from
3f5d3ca to
38fc5d4
Compare
Firestar99
force-pushed
the
shaders-graster-prep3
branch
from
38fc5d4 to
d36d0b4
Compare
|
I just noticed that I'll need them to be |
|
Actually, if they'd just default to black color if the Option is None, why have it be optional in the first place? |
Firestar99
force-pushed
the
shaders-graster-prep3
branch
from
5bab3de to
1b64e1f
Compare
|
@TrueDoctor are you sure you want to merge this, even though it'll break the UI for color selection of the two nodes? |
Firestar99
force-pushed
the
shaders-graster-prep3
branch
from
ea2e61b to
68f195c
Compare
|
I removed the one problematic change of |
|
Feel free to merge at your own discretion, if the change to colors is only temporary it is fine break the ui for now and fix that in a followup pr |
prep work for shaders, does not contain any shader stuff:
cargo b -p graster-nodes --no-default-featuresbeing broken on masterTable<Color>->Colorwhere only the first row is used anyway