Skip to content

Make AudioFrame trivially copyable, and remove extraneous warning delints#120840

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
Ivorforce:trivially-frameable
Jul 3, 2026
Merged

Make AudioFrame trivially copyable, and remove extraneous warning delints#120840
Repiteo merged 1 commit into
godotengine:masterfrom
Ivorforce:trivially-frameable

Conversation

@Ivorforce

Copy link
Copy Markdown
Member

What problem(s) does this PR solve?

AudioFrame is currently not trivially copyable.
While perhaps optimizable in later compiler stages, it is unnecessary to rely on this. It should be cleaned up.

Additional information

I also made Vector2 conversions explicit. Besides being a core goal, Vector2 uses real_t while AudioFrame uses float so there's real risk of data loss.

@Ivorforce Ivorforce added this to the 4.x milestone Jul 1, 2026
@Ivorforce Ivorforce requested review from a team as code owners July 1, 2026 21:41
@Ivorforce Ivorforce force-pushed the trivially-frameable branch 2 times, most recently from c17557f to 1b0eed7 Compare July 1, 2026 21:58
Comment thread core/math/audio_frame.h
@clayjohn

clayjohn commented Jul 1, 2026

Copy link
Copy Markdown
Member

Seems that this code was intentional. This PR basically brings us back to the state before #87006

@Ivorforce

Ivorforce commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Seems that this code was intentional. This PR basically brings us back to the state before #87006

It doesn't, it was l and r before; it's left and right now (still resolving the problem).

The original PR is a (small) performance hazard in hindsight (making this type not trivially copyable and not constexpr compatible) and should not have been merged in the form that it was.

@Ivorforce Ivorforce force-pushed the trivially-frameable branch 2 times, most recently from 196ef7b to 346d210 Compare July 3, 2026 10:57
Comment thread core/math/audio_frame.h Outdated
Comment thread core/math/audio_frame.h
@Ivorforce Ivorforce force-pushed the trivially-frameable branch from 346d210 to 5a88421 Compare July 3, 2026 11:06

@hpvb hpvb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The exact right amount of cursed.

@akien-mga akien-mga modified the milestones: 4.x, 4.8 Jul 3, 2026
@hpvb

hpvb commented Jul 3, 2026

Copy link
Copy Markdown
Member

I retract the below statement. After talking it through with @Ivorforce it seems obvious that the standard already guarantees for the cases we care about. Thanks for humoring me!

Details Looking at this a bit more (because I was reviewing #120845), I think maybe we should add one more static assert to this (And all of the changes in #120845) It is perhaps super pedantic, but what else am I here for:
static_assert(alignof(AudioFrame) >= alignof(float));

Like, in practice I don't think there's really any case where this can't hold, but if we're doing the static asserts anyway...

@Ivorforce

Ivorforce commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

We resolved the above comment in chat. Practically, the guarantee follows from the "first member is at offset 0" guarantee + the "every member is placed at its correct alignment" guarantee. This can only hold if the owning struct's alignment is at least that of each of its fields.

@Repiteo Repiteo merged commit 8af2abb into godotengine:master Jul 3, 2026
20 checks passed
@Repiteo

Repiteo commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Thanks!

@Ivorforce Ivorforce deleted the trivially-frameable branch July 3, 2026 15:44
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.

5 participants