Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ShapeTexture2D #84587

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

aXu-AP
Copy link
Contributor

@aXu-AP aXu-AP commented Nov 7, 2023

A possible implementation of godotengine/godot-proposals#1126

ShapeTexture2D
Similiar to other procedural textures like GradientTexture2D and NoiseTexture2D, but instead generates simple regular shapes.
Main use cases:

  1. Placeholder for prototyping, intended to be changed later.
  2. As particle texture, where simple shapes go a long way.

ShapeTexture2D generates a texture with specified colored shape with transparent background, with optional border. The shape can be any regular polygon or star (and circle is a polygon with enough vertices). Texture can also be antialiased by supersampling.

stars

As concerns were raised that this might not be what was wanted (the original proposal was quite vague on details), I decided to make a proof of concept draft.
This is mostly functional with following shortcomings:

  • Doesn't support corner rounding, which is often wanted for nicer results.
  • Border width is calculated in a simple, but incorrect way (sharper corners cause narrower borders). I was a bit lazy to do the math, but I will eventually.

scene/resources/shape_texture_2d.h Outdated Show resolved Hide resolved
scene/resources/shape_texture_2d.h Outdated Show resolved Hide resolved
scene/resources/shape_texture_2d.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips requested a review from a team November 7, 2023 18:21
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 7, 2023

@AThousandShips note that this is a work in progress and more of a proof of concept to test if this is an useful addition, I'm not sure it needs to be reviewed yet. I'll be correcting stuff which automatic checks catches however, to get some artifacts for easier testing.

@AThousandShips
Copy link
Member

Having it pass so people can test artifacts is kind of relevant :) that's why I reviewed, I'm not doing a review on functionality etc.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 7, 2023

Fair enough. Just didn't want to waste your time. Since you're here however, can you help with this. I'm not entirely sure what this means:
kuva
Is it just about spaces at the end of line, or does it want me to add indent there too and separate 4; to a separate line?

@AThousandShips
Copy link
Member

Both I think, there's trailing space, and also rules about indentation here which are confusing with clang

@aXu-AP aXu-AP force-pushed the shape-texture-2d branch 5 times, most recently from 20deb6e to 8a66a0e Compare November 7, 2023 20:26
Apply suggestions from code review
@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 7, 2023

IMO this type of implementation is excellent! Is this PR intended to be a proof-of-concept or something you'll want to get through? Just so I know if I should try helping (because I really want it in).

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 7, 2023

I'm not hard determined to get this through, but I do think it is a nice feature. Most projects need some circle textures etc., so creating them in-editor feels logical. Is it useful enough? I don't know, but hopefully we'll get some feedback now that it's testable.

So for now, it's more POC so that it could be tested in action. I also proposed making this similiar api for a vector based node, so most of the code can be simply copied over to implement that as well.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 7, 2023

As particles are one of main use cases I had in mind, I would love to get feedback from @QbieShay about this.

Is the feature set good for this use, or should it have something more? Or something less? Or is this just a fool's errand?

Also I forgot to thank @AThousandShips earlier, thanks for the help! You don't get thanked enough, but you seem to be everywhere ensuring that prs are high quality 👍

@AThousandShips
Copy link
Member

Thank you! Means a lot, I am happy my nit-picking is appreciated, and I've had enough experience with clang to know all the little folleys lol

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Some comments:

  • The border is rendered even if Border Width is set to 0. This results in a thin black outline being visible (especially when zooming in) as opaque black pixels. This is most visible when several sprites overlap. Example with GPUParticles2D node that has no ParticleProcessMaterial assigned:

image

This also goes the other way around if Fill Color is fully transparent but Border Color is opaque. Example with a transparent white fill color and a opaque red border color:

image

  • I'd say Antialiased should be enabled by default to match StyleBoxFlat behavior.

@QbieShay
Copy link
Contributor

QbieShay commented Nov 8, 2023

I think this is a great feature! We hae lots of people that make lowpoly/pixel art games that could benefit from this.

I think it'd be good if it was easily compatible with pixel art and lowpoly games (not sure if you need extra feature for pixel art)

For me personally, I try to avoid using built-in textures unless they are saved to disk. I work on big project and a lot of built-in textures where isnt necessary can start slowing down the project. I believe that, in order to get good results with these textures it's likely that they will be upscaled in size, so I think UX wise it'd be better if the creation of the texture prompts a file save dialogue like visual shader.

I hope this makes sense ^^

Bonus for me (no need to implement it in this PR): a gradient fill option for this texture type (border and fill separate from each other) and outline mode being either hard outline or outer glow.

@Calinou
Copy link
Member

Calinou commented Nov 8, 2023

For me personally, I try to avoid using built-in textures unless they are saved to disk. I work on big project and a lot of built-in textures where isnt necessary can start slowing down the project. I believe that, in order to get good results with these textures it's likely that they will be upscaled in size, so I think UX wise it'd be better if the creation of the texture prompts a file save dialogue like visual shader.

This would require saving the rasterized image, which is a destructive operation (you can't change its parameters afterwards, it's just an image like any other). It would also negate the file size benefit of using procedural textures.

Procedural textures are only rendered when they are first used in a scene, so having procedural textures in scenes that aren't loaded shouldn't impact project load times.

@QbieShay
Copy link
Contributor

QbieShay commented Nov 8, 2023

Procedural textures are only rendered when they are first used in a scene, so having procedural textures in scenes that aren't loaded shouldn't impact project load times.

This not very good imo, causes the game to randomly stutter if you raster at decent resolution.
I think saving the resource to disk by default creates a better workflow for the poor tech artist that will come later in the pipeline to fix stutters

@Calinou
Copy link
Member

Calinou commented Nov 8, 2023

This not very good imo, causes the game to randomly stutter if you raster at decent resolution.
I think saving the resource to disk by default creates a better workflow for the poor tech artist that will come later in the pipeline to fix stutters

Saving the .tres file alone isn't enough, you need to save the rasterized image alongside. This could be done in a non-destructive manner if we have a prerendering system similar to what fonts do already, but it's quite a lot of added complexity.

In the meantime, the ResourcePreloader node can be handy (you can have an autoloaded scene that has a ResourcePreloader root node).

@QbieShay
Copy link
Contributor

QbieShay commented Nov 8, 2023

What I'm saying is that if these textures are saved separately by default (even if they're still procedural), it will be easier for an eng/tech artist to write a tool (or modify the engine) in a way that at export-time the textures are rasterized. Otherwise if you embed them in the scene you'll have to go fish for textures everywhere, it's not practical.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 8, 2023

I think such caching option should be implemented on Texture level, so that it would be usable for noise and gradient textures as well. But likely it's an idea for another proposal.

@Calinou thanks for noticing bleeding problems!

@QbieShay: I think it'd be good if it was easily compatible with pixel art and lowpoly games

Do you mean using something like Bresenham's algorithm? Need to think how to approach this. Currently the texture is drawn by checking if each pixel is inside a polygon (fill or border polygon). Triangulation and filling might be faster and allow for better outline, but needs testing.

a gradient fill option for this texture type

Texture filling would be more flexible and less hassle to implement (no need to duplicate GradientTexture2D functionality in its entirety). We'll see if it will be performant enough.

outline mode being either hard outline or outer glow.

Again with current approach might be a bit hard, but I'll make a note of this.

Thanks for feedback!

@clayjohn
Copy link
Member

clayjohn commented Nov 8, 2023

Before this gets too far along, I want to express two concerns:

  1. This PR doesn't resolve Add a Shape2D node to draw 2D shapes godot-proposals#1126. The PR asks for a node to draw simple shapes. This PR is a tool for authoring textures. Textures come with a huge drawback in that they create aliasing, the texture size needs to be adjusted for scale and they add a lot of overdraw compared to using a shape.
  2. To me, this sort of thing should always be an addon. It is just way too specific and niche to be core. Godot is not an asset authoring tool and artists are always going to need more flexibility and control than we can provide. We should let the content authoring tools focus on content authoring while we focus on the game engine.

I think the idea is really cool. But this should be a GDExtension or a GDscript plugin instead of a part of the engine

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 8, 2023

I don't think we need to implement a "Shape2D node" like the proposal suggests to consider it solved -moreover it's a really vague proposal from 3 years ago. What matters is that the use cases are addressed. Whether the use case is through a Sprite2D with a ShapeTexture2D, or a Shape2D node, I don't think matters here.

Also I think the suggestion that it's in addon territory is shaky, to me it looks similar to the inclusion of primitive meshes, which are too useful to discard from core.

@Calinou
Copy link
Member

Calinou commented Nov 8, 2023

If GPUParticles2D could use meshes instead of sprites, then a vector-based Mesh2D with lower overdraw could be used (at the cost of pushing more triangles). For 3D, a separate flat 3D polygon primitive mesh would be required.

@QbieShay
Copy link
Contributor

QbieShay commented Nov 8, 2023

Alright, fair enough @clayjohn ! I came in the conversation late and I didn't really ask myself "should this be core"

However i still think out of this discussion we need to discuss the rasterization of procedural textures for export, which would benefit also a plugin like this one.

@PPillau
Copy link

PPillau commented Apr 20, 2024

So is this PR dead or can we expect this to get merged at some point? Would be very welcome for my current project...

@AThousandShips
Copy link
Member

@PPillau this is still being discussed based on the comments above, and still needs some work even if no changes to the approach was decided on by the team

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Apr 20, 2024

I think the general vibe is that this is not going to be part of the core, so I didn't pursue this further - the proposal that this is based on, altough approved at some level, is very vague in details. If somebody wants to turn this into a plugin feel free to do so.
I also tested making similiar feature in gdscript by generating svg snippets and loading them in via Image.load_svg_from_string which gave some promising results but had some problems like no control over antialiasing and white fringe around the shape (I think the latter might be a bug, needs more testing...).

@MewPurPur
Copy link
Contributor

and white fringe around the shape (I think the latter might be a bug, needs more testing...)

#82579

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Apr 20, 2024

@MewPurPur thanks, I actually just discovered the solution myself. I think the documentation for load_svg_from_string should hint about using fix alpha border if this is intented behavior (I wrote about it in the rocket chat just now - link ).

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Apr 22, 2024

Hey! For anybody interested in this feature, I implemented ShapeTexture2D as a gdscript addon - check the repo here. I'll submit it to the asset library soonish when I feel that the feature set/api is good.
@PPillau Since you said that you could use this in your current project, I would love to hear if the addon is suitable for your use cases :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs consensus
Development

Successfully merging this pull request may close these issues.

7 participants