Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh #88349

Calinou · 2024-02-14T23:29:08Z

If an invalid type is supplied (which can still be done from a script), a warning is printed (along with a workaround for ViewportTexture).

This also adds support for "negative" resource hints such as "Texture2D,-ViewportTexture" to exclude one or more subclasses from a class hint.

Testing project: test_decal_warning.zip

This closes Decal with MeshTexture appears broken (not visible, and also affects other decals) #77564.
See also AnimatedTextures not properly displaying using the Decal node #54506,
Decals using ViewportTexture are not updated #73400,
and ViewportTexture doesn't work within PointLight2D #75868.

Preview

Before	After

WARNING: ViewportTexture is not supported as a PointLight2D texture (/root/Node3D/PointLight2D). As a workaround, use `get_image()` on the ViewportTexture to assign the returned value to the PointLight2D texture property, but keep in mind this is a slow operation.
     at: set_texture (./scene/2d/light_2d.cpp:405)
WARNING: AtlasTexture is not supported as a Light3D projector texture (/root/Node3D/OmniLight3D).
     at: set_projector (./scene/3d/light_3d.cpp:207)
WARNING: MeshTexture is not supported as a Decal texture (/root/Node3D/Decal2).
     at: set_texture (./scene/3d/decal.cpp:66)

Calinou · 2024-02-14T23:31:05Z

scene/2d/light_2d.cpp

@@ -449,7 +476,7 @@ void PointLight2D::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("set_texture_scale", "texture_scale"), &PointLight2D::set_texture_scale);
 	ClassDB::bind_method(D_METHOD("get_texture_scale"), &PointLight2D::get_texture_scale);

-	ADD_PROPERTY(PropertyInfo(Variant::OBJECT, "texture", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), "set_texture", "get_texture");
+	ADD_PROPERTY(PropertyInfo(Variant::OBJECT, "texture", PROPERTY_HINT_RESOURCE_TYPE, "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D"), "set_texture", "get_texture");


This list of types is something we should probably reuse across in the editor a lot more, as there are a lot of situations where a type like CameraTexture or AtlasTexture is not allowed (only one or two of those at at time).

Maybe we should have a special syntax using - to denote exceptions, like Texture2D,-ViewportTexture,-AtlasTexture to allow anything that extends Texture2D except ViewportTexture and AtlasTexture.

I did not even know this was a feature. Concerning to me. It has been here for a while, yet PROPERTY_HINT_RESOURCE_TYPE is not documented well enough to showcase this.

Maybe we should have a special syntax using - to denote exceptions, like Texture2D,-ViewportTexture,-AtlasTexture to allow anything that extends Texture2D except ViewportTexture and AtlasTexture.

That sounds more scalable in case new texture types are added. I think the hint is only used in EditorResourcePicker, so it should be trivial to implement (the picker already unfolds the base type into inheriting classes).

Honestly I'm not sure I like this syntax to exclude unsupported types. We're excluding as many times as we'd have to explicitly include using the usual syntax.

And I think future proofing the addition of new types is not a good idea, as we don't know whether those new types would be supported. So it's better to state explicitly what we know we support. This would also make it easier possibly in the future to actually parse this hint and provide this information automatically in the class reference for example.

scene/3d/light_3d.cpp

scene/2d/light_2d.cpp

scene/3d/decal.cpp

scene/3d/light_3d.cpp

Mickeon

Not sure about "every frame", I'd say "frequently" but it's nitpicky.

Yippie

doc/classes/ViewportTexture.xml

doc/classes/Texture2D.xml

scene/3d/decal.cpp

KoBeWi · 2024-02-16T22:41:20Z

scene/2d/light_2d.cpp

+		if (Object::cast_to<AnimatedTexture>(*p_texture)) {
+			WARN_PRINT(vformat("AnimatedTexture cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<AtlasTexture>(*p_texture)) {
+			WARN_PRINT(vformat("AtlasTexture cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<CameraTexture>(*p_texture)) {
+			WARN_PRINT(vformat("CameraTexture cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<CanvasTexture>(*p_texture)) {
+			WARN_PRINT(vformat("CanvasTexture cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<MeshTexture>(*p_texture)) {
+			WARN_PRINT(vformat("MeshTexture cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<Texture2DRD>(*p_texture)) {
+			WARN_PRINT(vformat("Texture2DRD cannot be used as a PointLight2D texture (%s).", get_path()));
+		} else if (Object::cast_to<ViewportTexture>(*p_texture)) {
+			WARN_PRINT(vformat("ViewportTexture cannot be used as a PointLight2D texture (%s). As a workaround, assign the value returned by ViewportTexture's `get_image()` instead.", get_path()));
+		}


I think this should be replaced by a method that takes texture type and automatically includes it in the warning message. You can use is_class() to check class by String.
Not sure where to put the method though, perfectly it should be one for all 3 classes.

I've simplified the code somewhat for each class. I'm not sure where a generic method could be put so it can be reused across all 3 classes though.

doc/classes/ViewportTexture.xml

AThousandShips

LGTM

akien-mga · 2024-02-27T19:32:58Z

There are these errors to handle for the GDExtension compatibility checker:

Validate extension JSON: Error: Field 'classes/Decal/properties/texture_albedo': type changed value in new API, from "Texture" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".
Validate extension JSON: Error: Field 'classes/Decal/properties/texture_emission': type changed value in new API, from "Texture" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".
Validate extension JSON: Error: Field 'classes/Decal/properties/texture_normal': type changed value in new API, from "Texture" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".
Validate extension JSON: Error: Field 'classes/Decal/properties/texture_orm': type changed value in new API, from "Texture" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".
Validate extension JSON: Error: Field 'classes/Light3D/properties/light_projector': type changed value in new API, from "Texture2D" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".
Validate extension JSON: Error: Field 'classes/PointLight2D/properties/texture': type changed value in new API, from "Texture2D" to "CompressedTexture2D,PortableCompressedTexture2D,ImageTexture,CurveTexture,CurveXYZTexture,GradientTexture1D,GradientTexture2D,PlaceholderTexture2D,NoiseTexture2D".

I'm not sure how those hints are used in godot-cpp when there's multiple classes, cc @dsnopek

In theory the extension JSON should likely still have the common parent class as the expected type, which should be the one from the C++ method's signature. This hint is just for the editor.

raulsntos · 2024-02-27T22:05:24Z

I'm not sure how those hints are used in godot-cpp when there's multiple classes

AFAIK godot-cpp doesn't use the properties from the JSON. Since C++ doesn't have properties I guess there's nothing to generate, so they only generate the getter/setter methods.

In C#, we use the getter/setter method signature to determine the property type not the hint, so it doesn't affect us either.

Mickeon · 2024-03-14T19:44:38Z

I have not a clue as to why the check is still failing but I wanted to mention it as a reminder.

KoBeWi · 2024-03-14T21:34:32Z

If this is going to "break compatibility", it's better to do it properly. I was going to open a PR for that, but feel free to add this commit and use it to simplify the hints:
ff8f797
You can do e.g. "Texture2D,-ViewportTexture" to allow everything except ViewportTexture.

Calinou · 2024-05-21T22:38:38Z

Rebased and tested again following #84443, it works as expected.

akien-mga · 2024-05-22T06:37:55Z

We still have the GDExtension compat issue to figure out, which is blocking this.

dsnopek · 2024-05-22T15:42:28Z

Sorry for missing all the pings on this one!

I can definitely say this won't have any effect on godot-cpp: we don't use properties, because there isn't really an equivalent in C++ - we just use the getters and setters.

However, this may have an effect on bindings for languages that do have properties. That said, I would expect that in most cases, they'd use the return value type of the getter, rather than the property type hint in order to determine which type should be used for the property. There's plenty of properties which list multiple types, which would potentially make using that tricky.

If any bindings actually do use the property type, then the addition of the "minus types" would probably break things. But I personally feel like this sort of change is acceptable in a new minor release (like Godot 4.3) - we've made other small compatibility breaking changes to the format of extension_api.json in previous minor releases. But I don't think this is something that should be cherry-picked to Godot 4.2 or earlier.

clayjohn

Approved from the perspective of allowable types. I can't comment on the compatibility stuff or the changes to object etc.

Calinou · 2024-12-04T15:03:34Z

Rebased and tested again, it works as expected.

This now integrates #99959 by hiding non-manifold PrimitiveMeshes from the CSGMesh3D mesh property hint.

misc/extension_api_validation/4.3-stable.expected

akien-mga · 2024-12-16T10:00:30Z

Reposting my comment from the old thread above for visibility, as IMO that's still a blocker for doing these otherwise relatively straightforward changes:

Honestly I'm not sure I like this syntax to exclude unsupported types. We're excluding as many times as we'd have to explicitly include using the usual syntax.

And I think future proofing the addition of new types is not a good idea, as we don't know whether those new types would be supported. So it's better to state explicitly what we know we support. This would also make it easier possibly in the future to actually parse this hint and provide this information automatically in the class reference for example.

akien-mga · 2024-12-17T18:01:26Z

I amended the PR to finally fix the extension validation issue (same issue as in #90993, previous changes done in another stable version of Godot conflict with new changes to the same API, and the workaround is to list both in the current 4.3.stable.expected file).

I still have concerns about the new syntax in cases like Texture2D where we're excluding 7 types explicitly to keep 10, where we might want to just list the 10 types we know we support.

But let's not block it further, for other cases like CSGMesh it makes some sense (excluding 3 to keep 12).

akien-mga · 2024-12-17T19:40:56Z

Also added PointMesh to the list of primitives not supported by CSGMesh3D, as points are (I suppose?) non manifold.

…ure and CSGMesh3D mesh If an invalid type is supplied (which can still be done from a script), a warning is printed (along with a workaround for ViewportTexture). This also adds support for "negative" resource hints such as "Texture2D,-ViewportTexture" to exclude one or more subclasses from a class hint. Co-authored-by: Tomasz Chabora <kobewi4e@gmail.com>

akien-mga · 2024-12-17T22:08:10Z

Thanks!

Fix-up to godotengine#88349.

Calinou added bug topic:rendering topic:editor labels Feb 14, 2024

Calinou added this to the 4.3 milestone Feb 14, 2024

Calinou requested review from a team as code owners February 14, 2024 23:29

Calinou commented Feb 14, 2024

View reviewed changes

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch 2 times, most recently from e9a59e5 to 8cc36b8 Compare February 14, 2024 23:32

Mickeon reviewed Feb 14, 2024

View reviewed changes

scene/3d/light_3d.cpp Outdated Show resolved Hide resolved

AThousandShips reviewed Feb 15, 2024

View reviewed changes

scene/2d/light_2d.cpp Outdated Show resolved Hide resolved

scene/3d/decal.cpp Outdated Show resolved Hide resolved

scene/3d/light_3d.cpp Show resolved Hide resolved

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 8cc36b8 to 51a14a1 Compare February 15, 2024 17:37

Calinou requested a review from a team as a code owner February 15, 2024 17:37

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 51a14a1 to 66f9362 Compare February 15, 2024 18:10

Mickeon approved these changes Feb 15, 2024

View reviewed changes

doc/classes/ViewportTexture.xml Outdated Show resolved Hide resolved

doc/classes/Texture2D.xml Outdated Show resolved Hide resolved

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 66f9362 to 86f3949 Compare February 16, 2024 20:23

akien-mga reviewed Feb 16, 2024

View reviewed changes

scene/3d/decal.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Feb 16, 2024

View reviewed changes

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 86f3949 to 42fc2a7 Compare February 27, 2024 19:18

AThousandShips reviewed Feb 27, 2024

View reviewed changes

doc/classes/ViewportTexture.xml Outdated Show resolved Hide resolved

AThousandShips approved these changes Feb 27, 2024

View reviewed changes

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 42fc2a7 to 6e94973 Compare February 27, 2024 19:35

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 6e94973 to d2bf91f Compare February 28, 2024 16:24

Calinou requested review from a team as code owners February 28, 2024 16:24

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from d2bf91f to d651ccf Compare March 14, 2024 18:55

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 120f0c8 to d3344bf Compare May 21, 2024 22:38

clayjohn approved these changes May 22, 2024

View reviewed changes

akien-mga modified the milestones: 4.3, 4.4 Jun 12, 2024

akien-mga added the needs work label Jun 12, 2024

AThousandShips mentioned this pull request Aug 14, 2024

Can I use part of a sprite as the light texture? #95507

Open

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from d3344bf to 6697e70 Compare December 4, 2024 15:02

Calinou requested a review from a team as a code owner December 4, 2024 15:02

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from 6697e70 to e1abd54 Compare December 4, 2024 15:02

Calinou changed the title ~~Only allow valid types in Decal, Light3D projector and PointLight2D texture~~ Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh Dec 4, 2024

Calinou force-pushed the decal-light-projector-only-allow-valid-types branch from e1abd54 to bc18ed9 Compare December 4, 2024 21:29

KoBeWi removed the needs work label Dec 5, 2024

akien-mga reviewed Dec 12, 2024

View reviewed changes

misc/extension_api_validation/4.3-stable.expected Show resolved Hide resolved

akien-mga force-pushed the decal-light-projector-only-allow-valid-types branch from bc18ed9 to f648634 Compare December 17, 2024 17:56

akien-mga force-pushed the decal-light-projector-only-allow-valid-types branch from f648634 to a568528 Compare December 17, 2024 19:40

akien-mga force-pushed the decal-light-projector-only-allow-valid-types branch from a568528 to 7a04d85 Compare December 17, 2024 19:41

akien-mga approved these changes Dec 17, 2024

View reviewed changes

akien-mga merged commit 2c304a7 into godotengine:master Dec 17, 2024
20 checks passed

akien-mga added a commit to akien-mga/godot that referenced this pull request Dec 20, 2024

Decal: Fix crash when setting empty texture

ed07bf2

Fix-up to godotengine#88349.

akien-mga mentioned this pull request Dec 20, 2024

Decal: Fix crash when setting empty texture #100668

Closed

Calinou deleted the decal-light-projector-only-allow-valid-types branch December 21, 2024 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh #88349

Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh #88349

Calinou commented Feb 14, 2024 •

edited

Loading

Calinou Feb 14, 2024

Mickeon Feb 14, 2024

KoBeWi Feb 14, 2024

akien-mga Dec 12, 2024

Mickeon left a comment

KoBeWi Feb 16, 2024

Calinou Feb 27, 2024

AThousandShips left a comment

akien-mga commented Feb 27, 2024 •

edited

Loading

raulsntos commented Feb 27, 2024

Mickeon commented Mar 14, 2024

KoBeWi commented Mar 14, 2024

Calinou commented May 21, 2024 •

edited

Loading

akien-mga commented May 22, 2024

dsnopek commented May 22, 2024 •

edited

Loading

clayjohn left a comment

Calinou commented Dec 4, 2024 •

edited

Loading

akien-mga commented Dec 16, 2024

akien-mga commented Dec 17, 2024

akien-mga commented Dec 17, 2024

akien-mga commented Dec 17, 2024

Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh #88349

Only allow valid types in Decal, Light3D projector, PointLight2D texture and CSGMesh3D mesh #88349

Conversation

Calinou commented Feb 14, 2024 • edited Loading

Preview

Calinou Feb 14, 2024

Choose a reason for hiding this comment

Mickeon Feb 14, 2024

Choose a reason for hiding this comment

KoBeWi Feb 14, 2024

Choose a reason for hiding this comment

akien-mga Dec 12, 2024

Choose a reason for hiding this comment

Mickeon left a comment

Choose a reason for hiding this comment

KoBeWi Feb 16, 2024

Choose a reason for hiding this comment

Calinou Feb 27, 2024

Choose a reason for hiding this comment

AThousandShips left a comment

Choose a reason for hiding this comment

akien-mga commented Feb 27, 2024 • edited Loading

raulsntos commented Feb 27, 2024

Mickeon commented Mar 14, 2024

KoBeWi commented Mar 14, 2024

Calinou commented May 21, 2024 • edited Loading

akien-mga commented May 22, 2024

dsnopek commented May 22, 2024 • edited Loading

clayjohn left a comment

Choose a reason for hiding this comment

Calinou commented Dec 4, 2024 • edited Loading

akien-mga commented Dec 16, 2024

akien-mga commented Dec 17, 2024

akien-mga commented Dec 17, 2024

akien-mga commented Dec 17, 2024

Calinou commented Feb 14, 2024 •

edited

Loading

akien-mga commented Feb 27, 2024 •

edited

Loading

Calinou commented May 21, 2024 •

edited

Loading

dsnopek commented May 22, 2024 •

edited

Loading

Calinou commented Dec 4, 2024 •

edited

Loading