Skip to content
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 Godot's named colors as constants #601

Merged
merged 2 commits into from Feb 11, 2024

Conversation

fpdotmonkey
Copy link
Contributor

All named colors were included using values from Godot's color constant docs except for TRANSPARENT, which is instead TRANSPARENT_WHITE. This is to be more explicit, which here is useful when users are working with gradients or modulations of color. This creates a conflict with Color::from_string, which is documented with the colors.

from_string's documentation still references Godot's own color constants doc since that represents ground truth.

This is executed as a set of hard-coded values because I wasn't keen on spending the time to learn the code gen system. This may later be redone with code gen.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Feb 9, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-601

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!


/// Opaque white.
pub const WHITE: Color = Self::from_rgba(1.0, 1.0, 1.0, 1.0);
include! {"color_constants.rs"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the more common usage, also suggested by docs:

Suggested change
include! {"color_constants.rs"}
include!("color_constants.rs");

But I think here we don't even need include, a regular module would also work?
With the mod color_constants; being declared in mod.rs...

Not sure if we can influence this, but if somehow possible, it would be nice to list the constants after the methods in the docs, so that people see the more common used information on top. If needed, we can resort to include! but after the other impl.

You can use the bot's link as a check for the docs, it will be updated with a latency of a few minutes on each push 🙂

fpdotmonkey and others added 2 commits February 11, 2024 12:24
All named colors were included using values from Godot's color constant
docs except for `TRANSPARENT`, which is instead `TRANSPARENT_WHITE`.
This is to be more explicit, which here is useful when users are working
with gradients or modulations of color.  This creates a conflict with
`Color::from_string`, which is documented with the colors.

`from_string`'s documentation still references Godot's own color
constants doc since that represents ground truth.

This is executed as a set of hard-coded values because I wasn't keen on
spending the time to learn the code gen system.  This may later be
redone with code gen.

Signed-off-by: Jan Haller <bromeon@gmail.com>
@Bromeon
Copy link
Member

Bromeon commented Feb 11, 2024

Thanks a lot!

There was a small misunderstanding: color_constants should not be its own public module, it should just have an impl Color block with all the constants, extending the type. I changed that in your commit (marked as signed-off-by). Also added the CI fix as a separate commit.

@Bromeon Bromeon added this pull request to the merge queue Feb 11, 2024
Merged via the queue into godot-rust:master with commit c96f4f6 Feb 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants