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 GradientTexture2D #42855

Closed
wants to merge 1 commit into from
Closed

Add GradientTexture2D #42855

wants to merge 1 commit into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Oct 16, 2020

Based on #31655, co-authored-by @MarianoGnu.

godot_gradients

(yes, this is radial gradient texture used in a Light2D node, and animated with AnimationPlayer).

Godot proposal: godotengine/godot-proposals#1677.

Changes when comparing to #31655:

  • creates a new GradientTexture2D rather than modifying existing GradientTexture;
    • if we go this route, then GradientTexture is better renamed to GradientTexture1D;
  • create texture placeholder for when gradient is empty, similarly to NoiseTexture (NOTE: GradientTexture needs a similar fix).
  • fixed get_width and get_height virtual overrides (NOTE: GradientTexture needs a similar fix).
  • renamed enums to just Fill and Repeat, NO_REPEAT renamed to REPEAT_NONE (conform to existing naming convention).
  • moved member initialization from constructor to class declaration;
  • grouped fill_ and repeat_ properties in the inspector.
  • added PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT for gradient (doesn't work in current master, see Refactor auto-instantiation of Object properties in editor #43015);
  • written documentation;
  • duplicated the same icon used by GradientTexture (for now);
  • various code style fixes;

Not sure about fill_from and fill_to. Perhaps it would be better to rename this to fill_start_offset and fill_end_offset.

To-do

Test project:
gradient_texture_2d.zip

@Xrayez Xrayez requested a review from a team as a code owner October 16, 2020 16:07
@Chaosus Chaosus added this to the 4.0 milestone Oct 16, 2020
@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 16, 2020

@Chaosus
Copy link
Member

Chaosus commented Oct 22, 2020

@Xrayez this need to be rebased

@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 22, 2020

Rebased, fixed get_width and get_height virtual overrides in the process (same issue needs to be fixed for GradientTexture...)

See also test project attached in the original post.

@Chaosus
Copy link
Member

Chaosus commented Oct 22, 2020

It runs and seems to work correctly except for the light which does not apply the color mask even if it set up - I need to set it manually - probably a bug, could you look to fix it? :

grad_test

@Xrayez Xrayez requested a review from JFonS as a code owner October 22, 2020 14:07
Co-authored-by: Mariano Suligoy <marianognu.easyrpg@gmail.com>
@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 22, 2020

Fixed by creating texture placeholder, similarly to how NoiseTexture does it in master:

RID GradientTexture2D::get_rid() const {
	if (!texture.is_valid()) {
		texture = RS::get_singleton()->texture_2d_placeholder_create();
	}
	return texture;
}

Invalid textures were treated as bad by Light2D so no changes were applied, should be ok now.

The same limitation exists in GradientTexture (1D), but the fix is probably not needed there.

This is another change which differs current implementation from #31655, likely it wasn't completely obvious what changes were needed to make it work in master.

The editor still doesn't auto-instantiate the default Gradient, even with PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT. EDIT: should be fixed by #43010 or #43015.

@Chaosus Chaosus self-requested a review October 22, 2020 18:12
Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

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

Seems fine now :)

@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 25, 2020

I think this needs invert property, because if you use it as a radial texture for lights, then by default this will be:

Screenshot 2020-10-25 190807

In GDScript, in theory you could extend GradientTexture2D to override this to have properly defined:

@tool
class_name RadialLightTexture extends GradientTexture2D

func _init():
	fill = GradientTexture2D.FILL_RADIAL
	fill_from = Vector2(0.5, 0.5)
	fill_to = Vector2(0.5, 0.0)
	gradient = Gradient.new()
	# Invert by switching black and white.
	gradient.set_offset(0, 1.0)
	gradient.set_offset(1, 0.0)

But extending textures doesn't work because Texture is not registered as virtual class: #42830.

If you set those properties via inspector, you'll get this:

Screenshot 2020-10-25 191156

So, currently it takes a lot of steps to setup a radial texture like this... Mainly talking about use case described in godotengine/godot-proposals#1717, so perhaps adding invert property could help this process. Alternatively, see godotengine/godot-proposals#1724.

Automatically switching properties specifically for linear and radial types is also something to consider:

  1. For linear: fill_from = Vector2(0, 0), fill_to = Vector2(1, 0)
  2. For radial: fill_from = Vector2(0.5, 0.5), fill_to = Vector2(0.5, 0)

You only need to use either type of fill, so automatically switching those properties won't hurt anything, in my opinion.

@Xrayez
Copy link
Contributor Author

Xrayez commented Dec 13, 2020

The 3.2 version of this PR is now merged in Goost: goostengine/goost#43.

@Xrayez
Copy link
Contributor Author

Xrayez commented Apr 5, 2021

So, currently it takes a lot of steps to setup a radial texture like this... Mainly talking about use case described in godotengine/godot-proposals#1717, so perhaps adding invert property could help this process. Alternatively, see godotengine/godot-proposals#1724.

I've added LightTexture class in Goost which helps this use case significantly: goostengine/goost#60.

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.

3 participants