Implemented some additional API functions related to crate::core_type… #729

jacobsky · 2021-04-28T09:11:45Z

…s::Color

I needed some additional functionality from the Color class in godot and noticed that there were a lot of APIs that weren't accessible from rust.

This PR plumbs in the functions from godot and adds some tests that can be used to sanity check them.

This commit adds the API functionality and some basic sanity tests that I verified are passing in the engine. If additional tests are required, please let me know.
Signed-off-by: Jacobsky [email protected]

…s::Color Signed-off-by: Jacobsky <[email protected]>

ghost

Looks good. Thanks for the PR!

bors r+

729: Implemented some additional API functions related to crate::core_type… r=toasteater a=jacobsky …s::Color I needed some additional functionality from the Color class in godot and noticed that there were a lot of APIs that weren't accessible from rust. This PR plumbs in the functions from godot and adds some tests that can be used to sanity check them. This commit adds the API functionality and some basic sanity tests that I verified are passing in the engine. If additional tests are required, please let me know. Signed-off-by: Jacobsky <[email protected]> Co-authored-by: Jacobsky <[email protected]>

bors · 2021-04-28T09:34:44Z

Build failed:

ci

Bromeon · 2021-04-28T09:31:13Z

gdnative-core/src/core_types/color.rs

+        Color::from_sys(unsafe { (get_api().godot_color_darkened)(self.sys(), amount) })
+    }
+    #[inline]
+    pub fn from_hsv(h: f32, s: f32, v: f32, a: f32) -> Color {


Should this maybe be named hsv(), consistent with the rgb() constructor -- or rename the latter?

Also, I would add overloads for HSV and HSVA.

I'll go ahead and add those.

I think it's better to rename rgb() here: from_thing is a common naming pattern for constructors in Rust.

In my latest commit, I changed the name to hsv() and hsva() in in 41969dd.

I can change it to follow either pattern as they both make sense to me.

@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?

@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 adding from_* but deprecating the old methods is good for now.

@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.

gdnative-core/src/core_types/color.rs

ghost · 2021-04-28T10:02:45Z

Please also remember to fix clippy lints: you're missing a few #[inline]s.

gdnative-core/src/core_types/color.rs

Demindiro · 2021-04-28T10:49:34Z

~~u64 is better for optimizations, it's very likely that [u16; 4] will use either 4 registers or stack allocation vs just one register with u64.~~ it seems the compiler is actually capable of optimizing it.

As for the casting issue: there are to_le_bytes/to_be_bytes which are architecture independent.

Bromeon · 2021-04-28T10:56:10Z

Just replied again (devil-advocating my own statement).

I don't think optimization matters much, a compiler is likely to recognize static arrays of small sizes and treat them as double/quadruple words. To me it's more about clarity. There's not a huge difference between the two though.

[Edit]: As concluded here, let's go with [u8; 4] and [u16; 4] for now.

jacobsky · 2021-04-28T13:58:02Z

Completed changes related to #729 (comment) with commit 88d50d4

If you'd like any changes to the methods or anything looks odd, please let me know.

Bromeon · 2021-04-28T15:34:20Z

Not 100% sure about endianess. What is the memory layout of Godot's integers returned by to_*{32|64}?

That determines whether we can use transmute (aka C++ reinterpret_cast) at all...

jacobsky · 2021-04-28T22:29:12Z

That's a good question. There's not really any docs directly related to endianess, but the implementation is as follows

uint32_t Color::to_ARGB32() const {
	uint32_t c = (uint8_t)(a * 255);
	c <<= 8;
	c |= (uint8_t)(r * 255);
	c <<= 8;
	c |= (uint8_t)(g * 255);
	c <<= 8;
	c |= (uint8_t)(b * 255);

	return c;
}

And

uint64_t Color::to_ABGR64() const {
	uint64_t c = (uint16_t)(a * 65535);
	c <<= 16;
	c |= (uint16_t)(b * 65535);
	c <<= 16;
	c |= (uint16_t)(g * 65535);
	c <<= 16;
	c |= (uint16_t)(r * 65535);

	return c;
}

So it doesn't look like the engine considers endianess in their encoding. Unless I'm mistaken, that would means that it's encoded as the native endianess.

So I think this should work.

Bromeon · 2021-04-29T07:17:12Z

Thanks for looking up the implementation! So they use shift, which means bits can be ordered differently depending on endianess. The most significant bit (for to_ARGB32) is the alpha component, which is either at the beginning (big endian) or end (little endian) of the 4 bytes.

So I think this should work.

No. u32::to_ne_bytes() also uses mem::transmute() internally, which is simply reinterpreting the bit pattern.

Reinterpreting the bit pattern means whatever comes first in memory will be the alpha value (for ARGB). As written above, that alpha value can however be at the beginning or at the end in memory.

The correct, and most understandable way, would be to simply undo the shift operations.

jacobsky · 2021-04-29T07:33:16Z

@Bromeon thanks for the insight.

I also did some more research into these functions and it looks like these are intended for turning into pixel color data in various byte formats such as RGBA8888.

https://en.m.wikipedia.org/wiki/RGBA_color_model#RGBA8888

As this is the case, the pixel data is likely only going to be useful when attempting to write pixel data or texture data. Those would use the byte pool arrays under the hood as well.

https://docs.godotengine.org/en/stable/classes/class_poolbytearray.html#class-poolbytearray

Based on this, I'm starting to think that it would be better to return the u32/u64 directly and leave this up to users that need to manipulate these values.

For now, I'll go ahead and implement the unshift operations.

ghost · 2021-04-29T09:26:21Z

I also did some more research into these functions and it looks like these are intended for turning into pixel color data in various byte formats such as RGBA8888.

@jacobsky I think this is a good point. Perhaps it's best to leave the data in u32/u64 form if this is the intended use case.

Bromeon · 2021-04-29T10:37:08Z

Ah, ARGB32 is specifically a format that defines encoding in terms of most/least significant bits and not memory addresses, I see.
When converting it into byte arrays (PoolByteArray), we might run into the endianness issues again though, depending on what the destination format expects.

I was also under the impression that they use ints because arrays are so expensive in GDScript (dynamic + ref-counted).

Either way, let's document the methods. Otherwise all the knowledge we figured out in this thread will be lost, and people need to go through the same hoops again.

jacobsky · 2021-04-29T14:29:11Z

@Bromeon @toasteater I went ahead and just ported the conversions directly into the rust code as when calling the APIs there seemed to be an issue with the 64bit versions. From the outset it looks like the API wrapper is returning the value as an i32 which resulted in the wrong final values being pulled from the API.

I wrote a couple of test cases and while the code itself isn't exactly elegant, it seems to work just fine. I also went ahead and added some documentation about each of the methods with the endianess information in mind.

The changes are in 1988289

Bromeon

Thanks again for the changes! I've made a few comments, we can also wait for @toasteater to avoid any more back-and-forth modifications.

gdnative-core/src/core_types/color.rs

Bromeon · 2021-04-29T14:50:51Z

gdnative-core/src/core_types/color.rs

+    pub fn gray(&self) -> f32 {
+        // Implemented as described in godot docs
+        (self.r + self.b + self.g) / 3.0
+    }
+
+    #[inline]
+    pub fn inverted(&self) -> Color {
+        // Implementation as described in godot docs.
+        Color {
+            r: 1.0f32 - self.r,
+            g: 1.0f32 - self.g,
+            b: 1.0f32 - self.b,
+            a: self.a,
+        }
+    }


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.

I think it's fine to inline simple implementations on small types to avoid the FFI overhead. We do this for Vector2 and Vector3, and Color is essentially a Vector4. 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?

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 value 1.0 is mapped to 255. In other words, in a uniform distribution, it is infinitely improbable for a color to have a component with value 255 (in theory at least).

@Bromeon Uniform [0, 1] distributions do exist. The distribution used by Godot's RandomNumberGenerator class is one: https://github.com/godotengine/godot/blob/28f56e2cbf03a164741f2eade17f9515f887482c/core/math/random_pcg.h#L90 Nevermind, 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.

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).

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.

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.

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.

gdnative-core/src/core_types/color.rs

ghost

Most of the changes look good to me now. One little thing is that most of the methods can indeed take self instead of &self, since Color implements Copy and isn't really large (16 bytes). Most of Vector2 and Vector3 methods take self, too, and I don't think the situation is much different here. Otherwise, I agree with Bromeon's points. Thanks again for taking this so far and having patience with us!

gdnative-core/src/core_types/color.rs

jacobsky · 2021-04-30T12:46:13Z

Thank you for all of the insight. I think I managed to include all of the requested changes as of e1ffa8e. If you have any additional changes or see any issues, please let me know.

ghost

Looks great to me. Thanks for all the effort! Squash the commits and we should be good to go!

…ore_types::Color

jacobsky · 2021-04-30T13:22:24Z

@toasteater Alrighty. I went ahead and squashed it on commit c608e1a, I hope I did it correctly.

ghost · 2021-04-30T13:33:52Z

It's OK. Thank you!

bors r+

bors · 2021-04-30T13:49:51Z

Build succeeded:

ci

ghost · 2021-04-30T13:50:50Z

🎉 Sorry it took so long!

jacobsky · 2021-05-01T06:34:52Z

Not at all! I'm glad that it was fun learning about some of the inner-workings of godot/rust! Thank you for all of the help!

Implemented some additional API functions related to crate::core_type…

45f1279

…s::Color Signed-off-by: Jacobsky <[email protected]>

ghost approved these changes Apr 28, 2021

View reviewed changes

Bromeon requested changes Apr 28, 2021

View reviewed changes

Bromeon reviewed Apr 28, 2021

View reviewed changes

gdnative-core/src/core_types/color.rs Outdated Show resolved Hide resolved

Bromeon requested changes Apr 29, 2021

View reviewed changes

ghost reviewed Apr 29, 2021

View reviewed changes

gdnative-core/src/core_types/color.rs Outdated Show resolved Hide resolved

gdnative-core/src/core_types/color.rs Outdated Show resolved Hide resolved

ghost approved these changes Apr 30, 2021

View reviewed changes

squash! Implemented some additional API functions related to crate::c…

c608e1a

…ore_types::Color

jacobsky force-pushed the master branch from e1ffa8e to c608e1a Compare April 30, 2021 13:20

bors bot merged commit d95b8fa into godot-rust:master Apr 30, 2021

ghost mentioned this pull request May 1, 2021

Make a 0.9.4 release #733

Closed

7 tasks

Implemented some additional API functions related to crate::core_type… #729

Implemented some additional API functions related to crate::core_type… #729

Conversation

jacobsky commented Apr 28, 2021

ghost left a comment

Choose a reason for hiding this comment

bors bot commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobsky Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

jacobsky Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 28, 2021

Demindiro commented Apr 28, 2021 • edited Loading

Bromeon commented Apr 28, 2021 • edited Loading

jacobsky commented Apr 28, 2021

Bromeon commented Apr 28, 2021

jacobsky commented Apr 28, 2021 • edited Loading

Bromeon commented Apr 29, 2021

jacobsky commented Apr 29, 2021

ghost commented Apr 29, 2021

Bromeon commented Apr 29, 2021 • edited Loading

jacobsky commented Apr 29, 2021

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 29, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 29, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

jacobsky commented Apr 30, 2021

ghost left a comment

Choose a reason for hiding this comment

jacobsky commented Apr 30, 2021 • edited Loading

ghost commented Apr 30, 2021

bors bot commented Apr 30, 2021

ghost commented Apr 30, 2021

jacobsky commented May 1, 2021

jacobsky Apr 28, 2021 •

edited

Loading

jacobsky Apr 28, 2021 •

edited

Loading

Demindiro commented Apr 28, 2021 •

edited

Loading

Bromeon commented Apr 28, 2021 •

edited

Loading

jacobsky commented Apr 28, 2021 •

edited

Loading

Bromeon commented Apr 29, 2021 •

edited

Loading

ghost Apr 29, 2021 •

edited by ghost

Loading

ghost Apr 29, 2021 •

edited by ghost

Loading

jacobsky commented Apr 30, 2021 •

edited

Loading