-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
base: master
Are you sure you want to change the base?
Add ShapeTexture2D #84587
Conversation
@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. |
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. |
Both I think, there's trailing space, and also rules about indentation here which are confusing with clang |
20deb6e
to
8a66a0e
Compare
Apply suggestions from code review
8a66a0e
to
ee5567c
Compare
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). |
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. |
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 👍 |
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 |
There was a problem hiding this 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:
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:
- I'd say Antialiased should be enabled by default to match StyleBoxFlat behavior.
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. |
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. |
This not very good imo, causes the game to randomly stutter if you raster at decent resolution. |
Saving the In the meantime, the ResourcePreloader node can be handy (you can have an autoloaded scene that has a ResourcePreloader root node). |
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. |
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!
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.
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.
Again with current approach might be a bit hard, but I'll make a note of this. Thanks for feedback! |
Before this gets too far along, I want to express two concerns:
I think the idea is really cool. But this should be a GDExtension or a GDscript plugin instead of a part of the engine |
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. |
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. |
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. |
So is this PR dead or can we expect this to get merged at some point? Would be very welcome for my current project... |
@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 |
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. |
|
@MewPurPur thanks, I actually just discovered the solution myself. I think the documentation for |
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. |
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:
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.
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: