Implement Vector4, Vector4i, Projection built-in types. #63219

reduz · 2022-07-19T23:17:54Z

Implement built-in classes Vector4, Vector4i and Projection.

Two versions of Vector4 (float and integer).
A Projection class, which is a 4x4 matrix specialized in projection types.

These types have been requested for a long time, but given they were very corner case they were not added before.
Because in Godot 4, reimplementing parts of the rendering engine is now possible, access to these types (heavily used by the rendering code) becomes a necessity.

TODO:

Shaders should be changed to use these for exposed uniforms. Can be done in a subsequent PR.
Enums do not seem to be registered properly, need to check what is going on.

FAQ:

Q: Why Projection and not Matrix4?
A: Godot does not use Matrix2, Matrix3, Matrix4x3, etc. naming convention because, within the engine, these types always have a purpose. As such, Godot names them: Transform2D, Transform3D or Basis. In this case, this 4x4 matrix is always used as a Projection, hence the naming.

Bugsquad edit: This closes godotengine/godot-proposals#629.

Bugsquad edit: Supersedes #59970.

Calinou · 2022-07-19T23:22:45Z

Shaders should be changed to use these for exposed uniforms. Can be done in a subsequent PR.

This may be done in #59970 already, so it's worth checking that PR's code just in case.

core/math/vector4.h

aaronfranke · 2022-07-19T23:35:47Z

core/variant/variant.h

@@ -91,11 +94,14 @@ class Variant {
 		VECTOR3,
 		VECTOR3I,
 		TRANSFORM2D,
+		VECTOR4,
+		VECTOR4I,


I know it's arbitrary, but this order doesn't really make sense. Why should Transform2D go in-between Vector3 and Vector4? I think we should consider reordering the types in the enum, maybe logical grouping, maybe alphabetical.

For logical grouping it would make sense to have Vector2/2i/3/3i/4/4i first, then Plane/Quat(/Color?), then Rect2/2i/AABB, then Transform2D, Basis, Transform3D, Projection.

EDIT: Updated my suggestion, now all types with 4 elements are together.

Its together with the other 4 component types so to me it makes sense that way. Moving then is far too much work also, so won't do without a very good reason.

Upon further inspection, I think the problem is that TRANSFORM2D should go above VECTOR3. We could change this in a separate PR I guess.

Also keep in mind, while I guess this isn't a problem since we're still in Alpha and we accept breakage, that adding new types in the middle will segfault any existing GDExtension that assumes the old numbering.

clayjohn · 2022-07-19T23:54:42Z

For posterity can you comment briefly on the difference between this PR and #59970?

reduz · 2022-07-20T07:12:47Z

@clayjohn That one is only Vector4 and from my understanding it misses a few bits of the type registration in the core areas so it was not entirely correct. Because all the 3 types I added interact with each other, it was far easier to me to implement everything together.

That said, that PR still implements a few more things like shader interaction, which I did not. I need to add that.

nathanfranke · 2022-07-20T18:33:34Z

Should PackedVector4Array also be added? I don't see that added in variant.h. It would be helpful for shader uniform vec4 arrays.

bruvzg

A few mismatched argument names in the bindings.

core/variant/variant_call.cpp

bruvzg · 2022-07-20T20:51:44Z

Follow-up changes in the godot-cpp - godotengine/godot-cpp#793

BastiaanOlij · 2022-07-20T23:58:14Z

Love this @reduz , haven't got much to add that hasn't already been said. Agree with the names change to Projection, makes far more sense.

One thing I think we should discuss as a possibility is merging Color and Vector4, adding r,g,b,a as a union, and then having Color be a typedef of Vector4 just like Size2 is a typedef of Vector2.

aaronfranke · 2022-07-21T00:00:43Z

@BastiaanOlij Color always stores 32-bit floats, while Vector4 uses real_t, so it wouldn't be appropriate. It would make more sense to do this with Plane and Quaternion and Rect2, but I'm not in favor of doing this to any of them.

EDIT: Also, Color has things like automatic color space conversions, which can be confusing to work with.

BastiaanOlij

Love this change

BastiaanOlij · 2022-07-21T00:16:52Z

@BastiaanOlij Color always stores 32-bit floats, while Vector4 uses real_t, so it wouldn't be appropriate. It would make more sense to do this with Plane and Quaternion and Rect2, but I'm not in favor of doing this to any of them.

I guess it would indeed be overkill to change Color to real_t. My motivation here is because in shaders Color is a vec4 and interchangeable and there are a lot of functions that overlap.

From a usage point of view Color and Vector4 are different enough that on the game engine side it is definitely a defensible position that they should be separate classes if only to prevent confusion from users.

Implement built-in classes Vector4, Vector4i and Projection. * Two versions of Vector4 (float and integer). * A Projection class, which is a 4x4 matrix specialized in projection types. These types have been requested for a long time, but given they were very corner case they were not added before. Because in Godot 4, reimplementing parts of the rendering engine is now possible, access to these types (heavily used by the rendering code) becomes a necessity. **Q**: Why Projection and not Matrix4? **A**: Godot does not use Matrix2, Matrix3, Matrix4x3, etc. naming convention because, within the engine, these types always have a *purpose*. As such, Godot names them: Transform2D, Transform3D or Basis. In this case, this 4x4 matrix is _always_ used as a _Projection_, hence the naming.

nathanfranke · 2022-07-23T17:15:32Z

core/variant/variant.h

@@ -91,11 +94,14 @@ class Variant {
 		VECTOR3,
 		VECTOR3I,
 		TRANSFORM2D,
+		VECTOR4,


PackedVector4Array?

you can still use a regular array for this use case or PackedColorArray, so its not suuuuper important.

Changes done.

akien-mga · 2022-07-25T09:13:31Z

Thanks!

reduz requested review from a team as code owners July 19, 2022 23:17

Calinou added enhancement topic:core labels Jul 19, 2022

Calinou added this to the 4.0 milestone Jul 19, 2022

aaronfranke reviewed Jul 19, 2022

View reviewed changes

core/math/vector4.h Outdated Show resolved Hide resolved

aaronfranke reviewed Jul 19, 2022

View reviewed changes

reduz force-pushed the implement-vector4-projection branch from 596b048 to 900503c Compare July 20, 2022 10:23

reduz requested review from a team as code owners July 20, 2022 10:23

reduz force-pushed the implement-vector4-projection branch from 900503c to 1dba423 Compare July 20, 2022 12:15

reduz requested a review from a team as a code owner July 20, 2022 12:15

reduz force-pushed the implement-vector4-projection branch 4 times, most recently from ef913f3 to 49ccf4b Compare July 20, 2022 17:57

bruvzg previously requested changes Jul 20, 2022

View reviewed changes

core/variant/variant_call.cpp Outdated Show resolved Hide resolved

core/variant/variant_call.cpp Outdated Show resolved Hide resolved

bruvzg mentioned this pull request Jul 20, 2022

Add bindings for Vector4, Vector4i, Projection built-in types. godotengine/godot-cpp#793

Merged

BastiaanOlij approved these changes Jul 21, 2022

View reviewed changes

reduz force-pushed the implement-vector4-projection branch 5 times, most recently from cbf7273 to 440a66f Compare July 23, 2022 11:07

reduz force-pushed the implement-vector4-projection branch from 440a66f to 455c06e Compare July 23, 2022 12:00

nathanfranke reviewed Jul 23, 2022

View reviewed changes

akien-mga requested a review from bruvzg July 25, 2022 09:03

akien-mga approved these changes Jul 25, 2022

View reviewed changes

akien-mga merged commit 3084a48 into godotengine:master Jul 25, 2022

akien-mga mentioned this pull request Jul 25, 2022

Add Vector4 #59970

Closed

raulsntos mentioned this pull request Jul 25, 2022

Mono build fails with invalid literal suffix 'i' (C3688) #63445

Closed

qarmin mentioned this pull request Jul 26, 2022

Add support for testing Vector4 arguments godotengine/regression-test-project#71

Merged

Chaosus mentioned this pull request Jul 27, 2022

Fix incorrect conversion of default value for mat4 uniform in shaders #63522

Merged

Calinou mentioned this pull request Jul 27, 2022

Fix polyline stroke #63425

Closed

fire mentioned this pull request Jul 30, 2022

Allow passing Matrix and Vector4 to shaders godotengine/godot-proposals#258

Closed

geowarin mentioned this pull request Jul 30, 2022

Add C# resource export. #63126

Closed

cridenour mentioned this pull request Aug 11, 2022

MultiplayerSynchronizer fatally crashes when attempting to sync a PackedStringArray #64231

Closed

TechnoPorg mentioned this pull request Oct 11, 2022

Fully document the Projection class. #67254

Merged

raulsntos mentioned this pull request Jan 15, 2023

C#: Add missing Transform{2D,3D} and Basis constructors #71445

Merged

Calinou mentioned this pull request Jun 19, 2024

Support GLSL style initialization and functions for vectors godotengine/godot-proposals#4822

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Vector4, Vector4i, Projection built-in types. #63219

Implement Vector4, Vector4i, Projection built-in types. #63219

reduz commented Jul 19, 2022 •

edited by aaronfranke

Loading

Calinou commented Jul 19, 2022

aaronfranke Jul 19, 2022 •

edited

Loading

reduz Jul 19, 2022

reduz Jul 20, 2022

BastiaanOlij Jul 20, 2022

clayjohn commented Jul 19, 2022

reduz commented Jul 20, 2022

nathanfranke commented Jul 20, 2022

bruvzg left a comment

bruvzg commented Jul 20, 2022

BastiaanOlij commented Jul 20, 2022

aaronfranke commented Jul 21, 2022 •

edited

Loading

BastiaanOlij left a comment

BastiaanOlij commented Jul 21, 2022

nathanfranke Jul 23, 2022

reduz Jul 23, 2022

akien-mga commented Jul 25, 2022

Implement Vector4, Vector4i, Projection built-in types. #63219

Implement Vector4, Vector4i, Projection built-in types. #63219

Conversation

reduz commented Jul 19, 2022 • edited by aaronfranke Loading

Calinou commented Jul 19, 2022

aaronfranke Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

reduz Jul 19, 2022

Choose a reason for hiding this comment

reduz Jul 20, 2022

Choose a reason for hiding this comment

BastiaanOlij Jul 20, 2022

Choose a reason for hiding this comment

clayjohn commented Jul 19, 2022

reduz commented Jul 20, 2022

nathanfranke commented Jul 20, 2022

bruvzg left a comment

Choose a reason for hiding this comment

bruvzg commented Jul 20, 2022

BastiaanOlij commented Jul 20, 2022

aaronfranke commented Jul 21, 2022 • edited Loading

BastiaanOlij left a comment

Choose a reason for hiding this comment

BastiaanOlij commented Jul 21, 2022

nathanfranke Jul 23, 2022

Choose a reason for hiding this comment

reduz Jul 23, 2022

Choose a reason for hiding this comment

akien-mga commented Jul 25, 2022

reduz commented Jul 19, 2022 •

edited by aaronfranke

Loading

aaronfranke Jul 19, 2022 •

edited

Loading

aaronfranke commented Jul 21, 2022 •

edited

Loading