-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add GradientTexture2D #2 #53234
Add GradientTexture2D #2 #53234
Conversation
619cd5d
to
43cf288
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.
I did not go into the details of the implementation, but the API looks fine and the feature seems a good addition to me.
scene/resources/texture.cpp
Outdated
wd8[(x + (y * width)) * 4 + 0] = uint8_t(CLAMP(c.r * 255.0, 0, 255)); | ||
wd8[(x + (y * width)) * 4 + 1] = uint8_t(CLAMP(c.g * 255.0, 0, 255)); | ||
wd8[(x + (y * width)) * 4 + 2] = uint8_t(CLAMP(c.b * 255.0, 0, 255)); | ||
wd8[(x + (y * width)) * 4 + 3] = uint8_t(CLAMP(c.a * 255.0, 0, 255)); |
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.
Casting like uint8_t(...)
will trim the fractional part which means that with the current implementation only values >= 1.0
would result in 255
value (so for example 0.999
would result in 254
). I doubt that's desired. So probably it should be (using round()
as mentioned in the comment below would be better):
wd8[(x + (y * width)) * 4 + 0] = uint8_t(CLAMP(c.r * 255.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 1] = uint8_t(CLAMP(c.g * 255.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 2] = uint8_t(CLAMP(c.b * 255.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 3] = uint8_t(CLAMP(c.a * 255.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 0] = uint8_t(CLAMP(c.r * 256.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 1] = uint8_t(CLAMP(c.g * 256.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 2] = uint8_t(CLAMP(c.b * 256.0, 0, 255)); | |
wd8[(x + (y * width)) * 4 + 3] = uint8_t(CLAMP(c.a * 256.0, 0, 255)); |
It would divide <0.0, 1.0> range into 256 equal length ranges (up to the precision errors) so for example values from range (more or less) <0.99609375, 1.0> would result in 255 value.
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.
Yeah but this is how Image
does it currently.
Perhaps have to use round()
in those cases?: #47022 (comment) (it would be less efficient I presume).
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 would keep it like that at least for now since it is consistent with Image and GradientTexture(1D) and discuss/tackle this problem in a future PR (for all occurrences).
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.
Yeah but this is how
Image
does it currently.
Hmm, then maybe it's indeed a desired behavior? Not sure. But yeah, in this PR it can be made consistent with the rest of the code base then (and potential change could be made in a different PR).
Perhaps have to use
round()
in those cases?: #47022 (comment) (it would be less efficient I presume).
After rethinking using round()
would indeed be better then what I proposed here, it would result in a smaller average absolute "conversion error". And performance wise I guess it shouldn't be a big difference, it's probably just an additional + 0.5
instruction before the truncation (but it probably should be benchmarked).
Anyway, I agree with both of you, it's out of scope of this PR.
43cf288
to
e118505
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.
I did not go into details, but previous versions were approved by me.
|
e118505
to
d3b9023
Compare
scene/resources/texture.cpp
Outdated
image->create(width, height, false, (use_hdr) ? Image::FORMAT_RGBAF : Image::FORMAT_RGBA8); | ||
Color color; | ||
if (gradient->get_points_count() == 1) { | ||
color = gradient->get_color(0); | ||
} else { | ||
color = Color(0, 0, 0, 1); | ||
} | ||
image->fill(color); |
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.
image->create(width, height, false, (use_hdr) ? Image::FORMAT_RGBAF : Image::FORMAT_RGBA8); | |
Color color; | |
if (gradient->get_points_count() == 1) { | |
color = gradient->get_color(0); | |
} else { | |
color = Color(0, 0, 0, 1); | |
} | |
image->fill(color); | |
image->create(width, height, false, use_hdr ? Image::FORMAT_RGBAF : Image::FORMAT_RGBA8); | |
if (gradient->get_points_count() == 1) { | |
image->fill(gradient->get_color(0)); | |
} |
Or if black is preferred over transparent:
image->create(width, height, false, (use_hdr) ? Image::FORMAT_RGBAF : Image::FORMAT_RGBA8); | |
Color color; | |
if (gradient->get_points_count() == 1) { | |
color = gradient->get_color(0); | |
} else { | |
color = Color(0, 0, 0, 1); | |
} | |
image->fill(color); | |
image->create(width, height, false, use_hdr ? Image::FORMAT_RGBAF : Image::FORMAT_RGBA8); | |
image->fill(gradient->get_points_count() == 1 ? gradient->get_color(0) : Color(0, 0, 0, 1)); |
Just a suggestion
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.
Changed it to your two-line-approach, however I kept the braces around the condition of the ternary operator since that seems to be an unwritten rule of Godot's code style guidelines.
7920eaa
to
72944fe
Compare
Co-authored-by: Mariano Javier Suligoy <marianognu.easyrpg@gmail.com> Co-authored-by: Andrii Doroshenko <xrayez@gmail.com>
72944fe
to
cd37af4
Compare
Thanks! |
Anything against backporting it to |
Probably not, it doesn't change any existing code and I don't think it relies on any 4.0 exclusive features |
Would it be possible to have textured gradients? |
You can use If you have a concrete example to show, feel free to create a discussion at https://github.com/godot-extended-libraries/godot-ideas |
Co-authored-by:
Mariano Suligoy marianognu.easyrpg@gmail.com
Andrii Doroshenko xrayez@gmail.com
This PR is essentially rebased version of #42855 slightly modified. Thanks to @Xrayez and @MarianoGnu for their awesome work!
I created a new icon for the GradientTexture2D which fits better in the current icon style and ported goostengine/goost@c9ff44d over. In addition, high dynamic range is now supported via the
use_hdr
property (like with GradientTexture).Although this is already implemented in Goost I believe this should be a part of the vanilla engine since it is really nice to work with and accelerates the workflow immensely in many cases/makes some things possible which couldn't be achieved without high efforts before. (useful for Light2D, backgrounds, particles, etc.)
Implements godotengine/godot-proposals#1677, closes godotengine/godot-proposals#3240 (discussed via chat that the texture is enough)
Some examples/use cases:
Here is an animated example (the text infill is also dynamic):
Feedback is welcome :)
Also: Any thoughts on whether GradientTexture should be renamed to GradientTexture1D?