Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implemented some additional API functions related to crate::core_type… #729
Implemented some additional API functions related to crate::core_type… #729
Changes from 1 commit
45f1279
c608e1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this maybe be named
hsv()
, consistent with thergb()
constructor -- or rename the latter?Also, I would add overloads for HSV and HSVA.
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'll go ahead and add those.
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 think it's better to rename
rgb()
here:from_thing
is a common naming pattern for constructors in Rust.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.
In my latest commit, I changed the name to
hsv()
andhsva()
in in 41969dd.I can change it to follow either pattern as they both make sense to me.
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.
@toasteater @Bromeon after thinking this over, I was wondering if it would be possible to use the
from_*
naming pattern, doing so would likely break compatibility with anyone that is currently using color.To avoid breaking compability, I could mark leave the rgb() and rgba() methods in while marking them as deprecated. Does that sound reasonable?
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.
@jacobsky That sounds reasonable! FWIW, we're working on a 0.10 release on
master
, so breaking compatibility is acceptable here, although this is useful to note since we might want to make a 0.9.4 release too. I think addingfrom_*
but deprecating the old methods is good for now.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.
@toasteater Thanks for the feedback. I'll get to work on that. I noticed that there were a few clippy lints that failed when I added the deprecated to rgb() and rgba(), so I want to take a little more time to ensure that I don't miss any of those changes.
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.
These two methods have a simple implementation, but is there a reason to not call the GDNative API? Or where do we draw the border?
For the case with
to_abgr64
, where the wrong type is returned by GDNative, it clearly makes sense.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 think it's fine to inline simple implementations on small types to avoid the FFI overhead. We do this for
Vector2
andVector3
, andColor
is essentially aVector4
. I don't see how this is problematic. That does not mean that we need to actively go port everything -- it only means that it's fine and welcome to do so. Let's say that we draw the border at Color / Vector4, maybe?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.
Ok, I think that's a good boundary 👍🏼
And yes, let's leave the other methods as is for now, no need to change what's not broken.
The only small risk is that if a Godot method ever got a small change in semantics or bugfix, it wouldn't be carried over, but for most of the methods this is unlikely.
One subtlety I could imagine is how the
[0.0, 1.0]
float range is mapped to the integer interval[0, 255]
-- just multiplying by 255 does not create even bins. All values in[0.0, 1/255]
are mapped to 0, but only the exact value1.0
is mapped to 255. In other words, in a uniform distribution, it is infinitely improbable for a color to have a component with value255
(in theory at least).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.
@Bromeon
Uniform [0, 1] distributions do exist. The distribution used by Godot'sNevermind, I understand what you mean now. Still, multiplying by 255 is a common way to do this, and if it's consistent with the Godot implementation I think it should be fine.RandomNumberGenerator
class is one: https://github.com/godotengine/godot/blob/28f56e2cbf03a164741f2eade17f9515f887482c/core/math/random_pcg.h#L90There 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.
Absolutely. My point was more that if Godot ever decides to change how this is implemented, we will be deviating slightly and could cause subtle bugs for users.
But I don't think it's something worthwhile to worry about, we could even add tests for such cases (later, not in this PR).
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.
That's a good point. I implemented these by hand initially because they were trivial and I wasn't sure how to call the API. If there's no significant performance hit, it'd probably be better to call the api. I'll test it later when I'm back at my PC.
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.
No, I didn't mean you should change it. As toasteater said, it's OK in these cases 🙂
I think you've had enough obstacles in this PR already -- if we really feel this could become a problem one day, we can simply add regression tests where we compare the built-in vs. the API function.
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.
Thanks for all of the information/discussion. I'll work on implementing a basic regression test for the custom implementation for this type and include it in another PR a little later.