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

Improve comments in Color documentation #42710

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 11, 2020

Fixes. #42707. EDIT: This is now a subset of what it used to be, due to some of the constructor methods being temporarily removed. I will update this PR if they are added back first, or otherwise I will make a follow-up after they are added back.

Two main problems:

  • The comments referenced RGBA and HSV as if they were methods, but they are not.

  • The comments used 0-255 notation pretty much everywhere, which is not the typical way Godot colors are represented.

I fixed both of these problems by replacing comments with valid code examples required to reproduce the colors. In most places prefer the normal Color constructor, but in a few places show how it compares with Color8. For from_hsv specifically, explain in detail what's going on, since it may not be obvious otherwise. Also be specific about whether an example is equivalent to or just similar to, since in many cases the colors are not exactly the same.

All the comments should be valid code examples now, so Color(0.8, 0.9, 0.4) in the GDScript comments becomes new Color(0.8f, 0.9f, 0.4f) in the C# comments.

@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 11, 2020

To remove repetition and make it translatable I think that at least the leading comments can be moved out of code blocks and into the surrounding text.

Though, I'm not sure if there is the same issue with code blocks being untranslatable in the class ref...

@setanarut
Copy link
Contributor

setanarut commented Oct 15, 2020

@aaronfranke The word "profile" is used incorrectly (10 times). Word should be a "color model" or just a "color". The "color profile" and "color model" and "color space" mean different things.

wrong

Constructs a color from an RGBA profile using values between 0 and 1.

correct

Constructs a RGBA color using values between 0 and 1.

correct

Constructs a color object from RGBA color model using values between 0 and 1.

[codeblocks]
[gdscript]
var color = Color(0.2, 1.0, 0.7, 0.8) # Equivalent to RGBA(51, 255, 178, 204)
var color = Color(0.2, 1.0, 0.7, 0.8) # Similar to `Color8(51, 255, 178, 204)`
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 the point of the RGBA() pseudo code was to show that the four parameters encode red, green, blue and optionally alpha in order. Not sure the change to Color8() preserves this meaning, though it makes it valid code and not pseudo code.

Copy link
Member

Choose a reason for hiding this comment

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

Well looking at further examples, maybe not. It was just that whoever wrote the docs really preferred 8-bit I guess... So the change is fine

Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the parameters should already be conveyed via the constructor argument order above the code example, and it also shows up when writing the code in GDScript.

@akien-mga akien-mga merged commit 06ac406 into godotengine:master Nov 14, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement labels Nov 14, 2020
@aaronfranke aaronfranke deleted the color-doc-comments branch November 14, 2020 09:38
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants