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

move cursor decals to shader #538

Merged
merged 6 commits into from
Jan 10, 2025
Merged

Conversation

Xtarsia
Copy link
Contributor

@Xtarsia Xtarsia commented Nov 2, 2024

removes all decals, closes #534

Adds a crosshair, with a threshold setting in the advanced menu, closes #533

Consolidate uniforms to just brush and ring textures

Also fixed derivative rotations
Also added more agressive branching to texture lookups in get_material()

Todo:

  • use crosshair instead of cross_hair
  • rework timer to lerp editor_decal_color uniform alpha to 0.
  • crosshair opacity to match editor_decal_color alpha
  • apply color space correction when updating editor_decal_color uniform RGB values if in compatibility
  • ensure correct position for bush operations

@TokisanGames
Copy link
Owner

Great!

  • What do you think about taking care of Visibility ring around cursor when small #533 in this PR as well and adding a larger open crosshair or circle around the cursor when small (or always)?

  • Please remove UI.gd:_on_setting_changed():update_decal(). It's annoying to have it popup all the time when changing texture colors. I have it on mine but havn't pushed it yet.

  • Why do we need separate decals for the gradients? Are there any ideas or reasons for having a third decal? What do you think if instead of three open slots for textures, we specify them: 0= current brush, 1=ring, 2=crosshair/etc? Then in the shader we can name them such as use them by name rather than editor_decal[0]?

@TokisanGames TokisanGames added this to the 1.0 milestone Nov 2, 2024
@TokisanGames TokisanGames added the shaders Shader development label Nov 2, 2024
@Xtarsia Xtarsia marked this pull request as ready for review November 2, 2024 13:12
@Xtarsia
Copy link
Contributor Author

Xtarsia commented Nov 2, 2024

ok im fairly happy with the cross hair now.

size 0;
afar:
image
upclose:
image

size 50;
afar:
image
upclose:
image

threshold setting in advanced, defaults to 16
image

@Xtarsia
Copy link
Contributor Author

Xtarsia commented Nov 2, 2024

shorter line length feels better
image

Copy link
Owner

@TokisanGames TokisanGames left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. In my review copy I rebased it, then tested.

  • crosshair(s) seems to be most widely used as a single word. So all cross_hair references could drop the underscore.

  • I'd like to see the crosshair lines the same opacity as the brush so they match better.

  • I don't see the fade on timeout, though I see there's code in there. The only way to make it disappear is a single right click, but it reappears when the mouse moves. There are times when the camera is close and the cursor is big, and we want to adjust something in the inspector and see the results on the terrain w/o the decal. Currently the process is to wait until it disappears then adjust. We need either the timeout, or maybe a better alternative is to hide the cursor as soon as the mouse leaves the viewport, or both.

  • In this PR, with Paint mode, if you right-click-hold and move the camera, let go (cursor invisible), click-hold and paint, the cursor will appear at a wrong spot and paint, then jump to where it should be and continue painting.

  • Comptability mode, Raise 100% strength, white decal Brightness is way too bright. As well as slope, spray, foliage ring. The other colors are acceptable. Forward foliage ring is a bit bright as well.

  • I'm wondering if the lines should change thickness/size based on the brush size. 🤔

  • Can be rebased and squashed to one commit.

project/addons/terrain_3d/src/ui.gd Outdated Show resolved Hide resolved
project/addons/terrain_3d/src/ui.gd Show resolved Hide resolved
project/addons/terrain_3d/src/ui.gd Outdated Show resolved Hide resolved
@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 3, 2025

  • In this PR, with Paint mode, if you right-click-hold and move the camera, let go (cursor invisible), click-hold and paint, the cursor will appear at a wrong spot and paint, then jump to where it should be and continue painting.

This happens on main too. Its related to get_intersection() GPU mode problems.

@TokisanGames
Copy link
Owner

Given the new changes to the shader, these tasks could be addressed, if you're up for it:

  • Audit the shader comments where they say 3 lookups and make sure those are accurate for each section
  • Review shader design and offer updates to things that have changed, or new information that would be useful to devs
  • Ensure the tip on adding a custom map is still accurate.

@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 4, 2025

what I wrote in the docs might need some more work.

the last commit is a bit cheeky, but updates the demo textures, and applies some detiling to them. Also made the grass a bit smaller too.

@Xtarsia Xtarsia force-pushed the remove-decals branch 2 times, most recently from 0085d97 to d647d87 Compare January 8, 2025 16:56
@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 8, 2025

@TokisanGames Rebased, and hopefully finished.

@TokisanGames
Copy link
Owner

fixed derivative rotations

Looks great. Much more stable now.

added more agressive branching to texture lookups in get_material()

Fantastic

Addressed issues:

  • Show decals on modifier presses
  • Don't show decal on inspector settings change (eg uv scale), only on tool settings

@TokisanGames
Copy link
Owner

There are times when I get these errors starting up, closing a scene, or saving the code.

SCRIPT ERROR: Invalid assignment of index '0' (on base: 'Array[bool]') with value of type 'bool'.
          at: update_decal (res://addons/terrain_3d/src/ui.gd:321)
SCRIPT ERROR: Out of bounds get index '0' (on base: 'Array[Color]')
          at: @editor_decal_fade_setter (res://addons/terrain_3d/src/ui.gd:63)

@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 10, 2025

There are times when I get these errors starting up, closing a scene, or saving the code.

i guess these lines:

var editor_decal_position: Array[Vector2] = [Vector2(), Vector2(), Vector2()]
var editor_decal_rotation: Array[float] = [float(), float(), float()]
var editor_decal_size: Array[float] = [float(), float(), float()]
var editor_decal_color: Array[Color] = [Color(), Color(), Color()]
var editor_decal_visible: Array[bool] = [bool(), bool(), bool()]

arent initializing correctly?

might have to do

editor_decal_position.resize(3) in _ready() instead for each one.

@TokisanGames
Copy link
Owner

TokisanGames commented Jan 10, 2025

_ready and _enter tree are for UI.gd which starts at engine launch time.
This problem circumstances occurs opening and closing user scenes so needs to be addressed when we're aware of a new terrain, eg. UI.set_visible, and stop checking when plugin.terrain is invalid. This seems to be working.

It's strange that they are losing their size after being set though. Also strange that I didn't notice any errors until today, but I don't see any changes I did to these arrays.

@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 10, 2025

Edit: set visible not enough.. I dont like this but it works.

func update_decal() -> void:
	if not plugin.terrain:
		return
	editor_decal_color.resize(3)
	editor_decal_position.resize(3)
	editor_decal_rotation.resize(3)
	editor_decal_size.resize(3)
	editor_decal_visible.resize(3)
	mat_rid = plugin.terrain.material.get_material_rid()
	editor_decal_timer.start()
var editor_decal_fade: float :
	set(value):
		if (editor_decal_color.size() > 0):
			editor_decal_fade = value
			editor_decal_color[0] = Color(
				editor_decal_color[0].r,
				editor_decal_color[0].g,
				editor_decal_color[0].b,
				value
			)
			RenderingServer.material_set_param(mat_rid, "_editor_decal_color", editor_decal_color)

this seems to cover enough bases.

opening and closing Demo / NavigationDemo giving no errors.

possibly cleaner placement of that block than right at the top of supdate_decal() or even somewhere else?

@TokisanGames
Copy link
Owner

TokisanGames commented Jan 10, 2025

I have no issues with these changes. Do you get any errors with just these, and nothing in update_decal?

 var editor_decal_fade: float :
        set(value):
                editor_decal_fade = value
+               if not plugin.terrain:
+                       return
                editor_decal_color[0] = Color(

func set_visible(p_visible: bool, p_menu_only: bool = false) -> void:
                ...
                visible = p_visible
                toolbar.set_visible(p_visible)
                tool_settings.set_visible(p_visible)
+               if editor_decal_color.size() != 3:
+                       # These sizes get reset to 0 on user scene closes. No idea why
+                       editor_decal_position.resize(3)
+                       editor_decal_rotation.resize(3)
+                       editor_decal_size.resize(3)
+                       editor_decal_color.resize(3)
+                       editor_decal_visible.resize(3)
                update_decal()

@Xtarsia
Copy link
Contributor Author

Xtarsia commented Jan 10, 2025

Yes, if i have both the demo, and navdemo scenes open at the same time with terrain3d selected in both scenes, switch back and forth, and then close 1, I get errors again until i deselect and reselect the terrain3d node in the remaining scene.

@TokisanGames TokisanGames force-pushed the remove-decals branch 2 times, most recently from d4f8fc1 to 2c1ce97 Compare January 10, 2025 15:36
@TokisanGames TokisanGames merged commit 8c241c1 into TokisanGames:main Jan 10, 2025
14 checks passed
@TokisanGames
Copy link
Owner

Excellent work as always. Thank you.

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

Successfully merging this pull request may close these issues.

Move cursor decals to shader Visibility ring around cursor when small
2 participants