Core: Purge remaining unions in math structs#120845
Conversation
|
Isn't this major compat breaking in third party code? |
Only if they actually use it (which, since we don't internally, they're unlikely too a well, I'd wager). Also, we don't guarantee code compat paths for modules — code health trumps. GDExtensions are unaffected by this change. |
Repiteo
force-pushed
the
core/union-busting
branch
3 times, most recently
from
df88b3b to
78df2fd
Compare
Repiteo
force-pushed
the
core/union-busting
branch
from
78df2fd to
605e25c
Compare
| template <> | ||
| struct is_zero_constructible<AudioFrame> : std::true_type {}; | ||
|
|
||
| static_assert(offsetof(AudioFrame, left) == 0 * sizeof(float)); |
There was a problem hiding this comment.
we made some "style decisions" these on #120840
Can you make sure these all look the same, and that all of these contain the same comment?
I really want to make sure that if this ever breaks for anyone that the comment is right there to explain what happened.
There was a problem hiding this comment.
Implemented! Slightly tweaked the text so that all instances can reuse the same blurb & no longer exceeds a width of 100
| glBindFramebuffer(GL_FRAMEBUFFER, rt->fbo); | ||
|
|
||
| glClearBufferfv(GL_COLOR, 0, rt->clear_color.components); | ||
| glClearBufferfv(GL_COLOR, 0, &rt->clear_color.r); |
There was a problem hiding this comment.
I don't like this at all, I realize that that is perhaps not a great comment, but this is confusing.
The old code was way more readable. This should just work with &rt->clear_color I think. If we can't make that work reasonably then I don't know if we should be doing this for this type.
I'd be way happier with an explicit cast, or even a method that does something sensible that is always inline.
But this I hate with the passion of a thousand suns.
There was a problem hiding this comment.
What I mean to say is, that the UB fuckery should be entirely hidden inside the class. Now we're telling the outside world as well that &color.r++ == &color.b
Maybe that's a bit more explanatory than my previous comment. :)
There was a problem hiding this comment.
Maybe something like this?
float *as_float4_buffer() {
return &r; // encapsulate the evil inside the function
}
glClearBufferfv(GL_COLOR, 0, &rt->clear_color.as_float4_buffer());
There was a problem hiding this comment.
I'd be 100% fine with that!
Alternatively maybe a operator float(&)[4]() that should be pretty safe, since it's an array type it won't just magically turn into a bool either.
There was a problem hiding this comment.
It doesn't appear like there's any way to make a return value for a carray without actually referencing a carray in some form. In the interest of keeping it simple, I opted for as_float4_buffer()
There was a problem hiding this comment.
In the interest of keeping it simple, I opted for as_float4_buffer()
Perfect! My dislike for that is measurable in a low 100s amount of joules.
Repiteo
force-pushed
the
core/union-busting
branch
from
605e25c to
f60a476
Compare
Repiteo
force-pushed
the
core/union-busting
branch
from
f60a476 to
60a16ad
Compare
5 participants
AudioFrametrivially copyable, and remove extraneous warning delints #120840What problem(s) does this PR solve?
Mirrors the above goal to make
AudioFrametrivially copyable; now extended toVector2,Vector2i,Vector3,Vector3i,Vector4,Vector4i,Quaternion, andColorAdditional information
A few areas of code were relying on
Colorarray access directly, but were able to be worked around by reinterpreting the struct itself as a container of floats (which, functionally, it is). Beyond that, the bracket accessors were all madeconstexprand given properDEV_ASSERTchecks if they weren't already present