search

godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add a built-in outline property to CanvasItemMaterial #2835

Open MaaaxiKing opened 3 years ago

MaaaxiKing commented 3 years ago

Describe the project you are working on

Any

Describe the problem or limitation you are having in your project

I always have to create a Shader (GDQuest on YouTube) for having Outlines on my Sprites, but in fact, this is commonly used. Moreover, outlines are provided on Font, so why not here?

Describe the feature / enhancement and how it helps to overcome the problem or limitation

You would not always need to create a shader for that.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

It would just be a property with thickness, color, mode (inner, outer, both), antialiasing, ...

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, not a few

Is there a reason why this should be core and not an add-on in the asset library?

Yes, several people use outlines.

Of course, it should also be provided for 3D.

Calinou commented 3 years ago

See also https://github.com/godotengine/godot-proposals/issues/96 and https://github.com/godotengine/godot-proposals/issues/1972.

This can be achieved with a shader. The issue with providing this as a built-in feature is that extending the built-in CanvasItem shader has a cost, both in terms of performance and maintenance.

My opinion is that we should improve the discoverability of asset library shaders from the editor instead.

Moreover, outlines are provided on Font, so why not here?

Font outlines are generated by FreeType on the CPU and don't require a shader to be rendered. However, there is no such equivalent for general-purpose sprites which may have varying levels of transparency.

SilencedPerson commented 3 years ago

See also #96 and #1972.

This can be achieved with a shader. The issue with providing this as a built-in feature is that extending the built-in CanvasItem shader has a cost, both in terms of performance and maintenance.

In regards to performance, wouldn't having BasicCanvasItemMaterial and AdvancedCanvasItemMaterial for less performant tasks be beneficial?

Calinou commented 3 years ago

In regards to performance, wouldn't having BasicCanvasItemMaterial and AdvancedCanvasItemMaterial for less performant tasks be beneficial?

It could work, but again, the AdvancedCanvasItemMaterial would likely grow without bound if we keep adding new features to it. We are already running into a similar issue with StandardMaterial3D (SpatialMaterial in 3.x) which has a lot of features that are seldom used, yet take up precious register space in the shader.

SilencedPerson commented 3 years ago

Then maybe adding specialized nodes to the VisualShader editor could be a solution. I do feel that the current implementation is way too low level to be of any use to users who don't know how to write their own shaders anyway.

Calinou commented 3 years ago

Then maybe adding specialized nodes to the VisualShader editor could be a solution. I do feel that the current implementation is way too low level to be of any use to users who don't know how to write their own shaders anyway.

This can be done via VisualShader plugins such as ShaderV :slightly_smiling_face:

MaaaxiKing commented 3 years ago

wouldn't having BasicCanvasItemMaterial and AdvancedCanvasItemMaterial for less performant tasks be beneficial?

Uh, I hate that. The same thing is with RichTextLabel and Label, there is a proposal for combining them into one and several users including me and also contributors liked it. Maybe it would be better for performance, but it is extremely user-unfriendly!

SilencedPerson commented 3 years ago

This can be done via VisualShader plugins such as ShaderV 🙂

That's actually illustrates my point very well. I would argue that having to rely on a plugins is not a good idea to present the feature.

Uh, I hate that. The same thing is with RichTextLabel and Label, there is a proposal for combining them into one and several users including me and also contributors liked it. Maybe it would be better for performance, but it is extremely user-unfriendly!

I agree with that proposal but there is huge reason it was allowed in the first place, performance is not as big of a concern unlike when changing implementation of CanvasItemShader.