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

Improve GradientTexture to allow more advanced types of fills #31655

Closed
wants to merge 1 commit into from

Conversation

MarianoGnu
Copy link
Contributor

@MarianoGnu MarianoGnu commented Aug 25, 2019

Pull Request sponsored by IMVU

This PR allows to create different linear and radial 2D textures with repeat and mirror options, while keeping the previous property values as default (2048px width, 1px height and left to right linear fill). Some examples bellow:
image
image
image
image

Bugsquad edit: This closes #18387.

@MarianoGnu
Copy link
Contributor Author

clang format is wrong, i forgot to install the hooks

@MarianoGnu
Copy link
Contributor Author

force-pushed to fix formatting

@akien-mga
Copy link
Member

You should also bind the enums in _bind_methods.

If @reduz agrees with the feature, I'd ask you to add the matching documentation too (run godot --doctool . to generate the template).

@MarianoGnu
Copy link
Contributor Author

bound the constants and ran doctool, but i didn't get any diff related to documentation, and the new properties was already visible in app

@akien-mga
Copy link
Member

bound the constants and ran doctool, but i didn't get any diff related to documentation, and the new properties was already visible in app

You probably ran doctool wrong, since you bind new methods, there should be new documentation to fill. You should:

  • Compile the PR
  • Then run bin/your.godot.bin --doctool . (the dot at the end is relevant)

@reduz
Copy link
Member

reduz commented Aug 27, 2019

I don't see much of an use case for this at all, and it makes using the class more complex for its intended usage, which is particle systems, visual shaders, color correction, etc.

@MarianoGnu
Copy link
Contributor Author

I need to use Gradients on UI nodes. I cannot have rounded shape on an styleboxtexture respecting the gradient. So my only choice is to mix a white stylebox flag with a light2D
styleboxflat doesn't have UV afaik, and even then wont match, because it would be stretched
and fragment or lighting shader wont affect a Light2D node.
So i combine the StyleBoxFlat with Light2D in MASK mode and gradient texture
yes, i can make the gradient with photoshop but: 1. i'm not good with visual edition programs and asking an artist to do it is slow because of timezones and 2. a png file will weigh more than a couple lines of plain text
i have had many usecases where mixing GradientTexture with UI was difficult to do, and this helped me a lot

@reduz
Copy link
Member

reduz commented Aug 27, 2019

You can make your own StyleBox class if you want, it's actually pretty easy

@reduz
Copy link
Member

reduz commented Aug 27, 2019

Or actually, maybe you can't and we should make sure you can.

@Calinou
Copy link
Member

Calinou commented Aug 27, 2019

At the very least, making the GradientTexture height adjustable and rotatable would be helpful for prototyping, as you can use them as a replacement for texture files or good old icon.png. See #29748.

@ghost
Copy link

ghost commented Aug 28, 2019

With the height option it would be nice as well for making Light2D masks without having to leave the editor.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 28, 2019

Gradient textures are very useful for TextureProgress, but I was always bothered by them not having height (and it was pretty much the only case when I used rect_scale).

@TheDuriel
Copy link
Contributor

TheDuriel commented Aug 30, 2019

2D Gradient Texture are also very useful as shader input. It eliminates the need to painstakingly create them in an external tool, which are not always equipped for the task.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 8, 2019

I was seeking for an easy way to create sky-like horizon gradients in 2D (which can be repeated with ParallaxLayer/Background), and this would fit nicely.

@Byteron
Copy link
Contributor

Byteron commented Oct 11, 2019

I want this.

@aaronfranke
Copy link
Member

@MarianoGnu Is this still desired? If so, it needs to be rebased on the latest master branch. Since this PR is a feature proposal, you should consider opening a proposal which explains example use cases and how this approach will solve the problem better than other approaches.

Otherwise, abandoned pull requests will be closed in the future as announced here.

@SekoiaTree
Copy link
Contributor

If this gets closed, I'm willing to pick it up.

@Extarys
Copy link

Extarys commented Sep 3, 2020

@MarianoGnu @SekoiaTree Newbie two cents: I started playing with Godot last month to know the basics. I believe having only 1 possible gradient direction like right now is useless. Users seeing they can do so will be left disappointed (like I was). It's a teasing feature, it's there but quite incomplete. I'm a newbie so I just want to play with godot and making squares etc and not spend time trying to make assets in GIMP/Inkscape just to learn a few things. The gradients made it possible for me to just play inside Godot and not get distracted.

I hope this will be implemented. Cheers! 🍻

@aaronfranke
Copy link
Member

If anyone would like to pick this up, they are welcome to. PRs should ideally be rebased frequently to avoid conflicts and to ensure they still work. The author of this PR has been inactive for a long time.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 4, 2020

I don't see much of an use case for this at all, and it makes using the class more complex for its intended usage, which is particle systems, visual shaders, color correction, etc.

@reduz, the current class is more like HorizontalGradientTexture, while the proposed class would be a GradientTexture indeed, so perhaps we just need two separate implementations at this point, or blame wrongly picked name for the class, because it just asks for those features, but currently the purpose of this class is certainly misleading for most users.

Do you know that GradientTexture used to be called ColorRamp in Godot <2.1 (while not being a texture)? Perhaps we just ended up with a bad rename... So perhaps lets rename it to ColorRampTexture and implement an actual GradientTexture with this PR? Linking #16863. 😃

Same for GradientColorRamp...

By the way, there's a similar class called CurveTexture, which is also one-dimensional and I suspect exists for similar described usage above (yet poorly documented as well).

I mean, particle systems, visual shaders, color correction are a bit too corner case for beginners, and "Gradient" is a more common and intuitive term which does not deserve to describe the intended usage of the existing class as in 3.2.

@SekoiaTree
Copy link
Contributor

Well, I just made finished another project, so I'll pick this up. I'll probably just make a completely new class and rename the old one to ColorRamp like @Xrayez suggested.

@akien-mga
Copy link
Member

Well, I just made finished another project, so I'll pick this up. I'll probably just make a completely new class and rename the old one to ColorRamp like @Xrayez suggested.

Well it was renamed for a reason. Please make sure to have the approval of core developers before spending time on such changes which might be rejected.

@TheDuriel
Copy link
Contributor

In that case pinging @reduz who did the intial rename, and @akien-mga who merged karroffels PR for finishing the rename.

c5dcbeb Suggests to me that ColorRamp ended up being renamed cuz the header was Gradient. Not because of deep semantic/functional reasoning.

Personally I'd be fine with a ColorGradient1D and Gradient2D. It also allows a Gradient3D to be added if someone would ever want that.

We shouldnt really touch CurveTexture with this discussion, its use case isnt graphics but maths. (it is necessary for it to be a "texture" only for use as a shader parameter)

@SekoiaTree
Copy link
Contributor

@akien-mga yes, of course, sorry.

@akien-mga
Copy link
Member

c5dcbeb Suggests to me that ColorRamp ended up being renamed cuz the header was Gradient. Not because of deep semantic/functional reasoning.

That's the other way around, ColorRamp was renamed to Gradient, but @reduz didn't bother renaming the file. I did it.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 4, 2020

Personally I'd be fine with a ColorGradient1D and Gradient2D. It also allows a Gradient3D to be added if someone would ever want that.

Reminds me of animation nodes/resources (see AnimationNodeBlendSpace1D for instance), so yeah makes sense to go for that route.

@MarianoGnu
Copy link
Contributor Author

Rebased the PR on top of master, also cherry picked for 3.2 and made compatibility changes at #41774

@tavurth
Copy link
Contributor

tavurth commented Apr 4, 2021

Just ran into this issue. Thank you for the work!

For now I'm just rotating the UV coordinates in a simple shader:

shader_type canvas_item;

void fragment() {
  COLOR = texture(TEXTURE, vec2(1.-UV.y, UV.x));
}

@Xrayez
Copy link
Contributor

Xrayez commented Apr 4, 2021

Note that I've also been working on this in #42855 (adds a separate class for this), based on this comment: #31655 (comment).

@fire
Copy link
Member

fire commented Apr 30, 2021

#42855 appears to be the success of this pr. So if that's correct, we should migrate to that pr and close this one.

@CsloudX
Copy link

CsloudX commented Aug 11, 2021

close this because it implement in goost ???!!!

@Xrayez
Copy link
Contributor

Xrayez commented Aug 11, 2021

@CsloudX as has been recommended by reduz, pull requests for new features should not be created. However, both this and #42855 have a common proposal at GIP, see godotengine/godot-proposals#1677.

If you'd really like to see this feature directly in Godot and not just in Goost, people must provide their own real-life use cases, and any limitations you stumble upon "must not be a speculation", as reduz says. By "real-life" I mean that you must have a project you're working on where you cannot proceed effectively without this feature. I hope this clarifies Godot's current development approach somewhat.

close this because it implement in goost ???!!!

This PR is like two years old, so I totally understand the decision to outright close the PR at its state by the author, and it can always be re-opened. If there's no decision making process going on, a person does not have to wait forever in the seek of feedback, it makes perfect sense to move on, don't you think? So, it's not necessarily because this feature is available somewhere else, it's because of lack of decision-making process regarding this feature.

But again, linking a proposal for this feature: godotengine/godot-proposals#1677. Hopefully, proposals will be thoroughly looked at once Godot 4.0 is out, I'd just like to say that ignoring (intentional or not) is not a solution to solve a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radial gradients