godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.88k stars 21.03k forks source link

NoiseTexture2D seamless not working #71996

Open beev1s opened 1 year ago

beev1s commented 1 year ago

Godot version

Godot 4.0 Beta 15

System information

Windows 10

Issue description

Scrolling a seamless 2D noise texture in 2D does not work as the texture does not appear to be seamless. The visual shader is the same between Godot 3.5.1.stable and Godot 4.0 Beta 15. The NoiseTexture is supposed to be moved by 0.5 in all examples.

BrokenScrollingNoiseTexture Broken in 2D

ScrollingNoiseTextureIn3D Works in 3D on a planar mesh

WorkingScrollingNoiseTexture Works in Godot 3.5.1.stable

Steps to reproduce

Open Scrolling2DNoiseTexture.tscn in the appended example

Minimal reproduction project

Broken2dSeamlessNoiseTextures.zip

clayjohn commented 1 year ago

Do you have repeat enabled on your CanvasItem?

beev1s commented 1 year ago

I did not not, though enabling it did not change anything

clayjohn commented 1 year ago

It looks like the repeat property is not getting correctly set in the VisualShader then. This isn't an issue with the NoiseTexture itself as the repeat mode is set within the shader.

aXu-AP commented 1 year ago

Also doesn't work using Texture2DParameter, which gives more options. I presume setting repeat to default would get it's value from CanvasItem.texture_repeat or project setting rendering/textures/canvas_textures/default_texture_repeat, but it seems to be disabled even if those are enabled. However, setting Texture2D as texture gets right values from the node. In the example below, noise texture is set as polygon's texture, and it repeats as it was set in node's properties. The icon texture however, doesn't repeat unless explicitly set to repeat from the shader itself.

visual_shader

This behaviour isn't only in visual shaders. In glsl shader the difference is following:

uniform sampler2D tex1 : repeat_disable; # Explicitly disabled
uniform sampler2D tex2; # By default disabled anyway

In 3.x this doesn't cause problems since repeat is set by texture basis on importing. In 4.x is there any way to change filter & repeat without changing shader code itself?

KoBeWi commented 1 year ago

I think this is intended behavior. The filter/repeat in CanvasItems apply only to the node's own texture (e.g. texture in Sprite2D). For shader uniforms you need to use the hint.

aXu-AP commented 1 year ago

Exactly, this was my understanding as well. However, I think it is an oversight since there's no way to change filtering/repeat flags from the editor without touching shader code. In a sense, it is a regression since in 3.x you could do it by changing texture import settings. This of course doesn't affect StandardMaterial3D, which does have flags for filtering/repeat.

miyanokomiya commented 1 year ago

Workaround for someone in the future

image

Chaosus commented 1 year ago

To fix it, you can enable the repeat mode on the TextureParameter node:

image

miyanokomiya commented 1 year ago

I found repeat hint. That's better than https://github.com/godotengine/godot/issues/71996#issuecomment-1492774431 uniform sampler2D noise: repeat_enable;

Chaosus commented 1 year ago

I found repeat hint. That's better than #71996 (comment) uniform sampler2D noise: repeat_enable;

This issue is about visual shader and not about common shaders, and that flag is already been used by that check box.

DasGandlaf commented 1 year ago

To reiterate, there is some bug going on, afaik. @Chaosus You've set repeat enabled for the texture in the scene. How can you do so, for a texture which only exists in the shader? Like so: (repeat is enabled in project settings): image Once you add texture 2d parameters: image

Did anyone find a fix, even if it's just a workaround?

Chaosus commented 1 year ago

@DasGandlaf If you connect the sampler port to the Texture2D node you should also change the source to SamplerPort at the first drop-down list (red warning at the bottom of the node is showing if you forgot to do it)...

DasGandlaf commented 1 year ago

@Chaosus Yes, but changing it to sampler port, uses the texture from the scene. The one that was defined in the node.

I hope this demonstrates the problem correctly: image

Using SamplerPort as the source, it uses the texture from the scene. But I want to set repeat on the noise, which I have imported from the FileSystem.

Anyways, I do appreciate your help.

Chaosus commented 1 year ago

@DasGandlaf Then simply replace that texture to the NoiseTexture in the inspector.

DasGandlaf commented 1 year ago

I don't think that is a fix. It wouldn't work, If I need two different scrolling, repeating noise textures for example, as I can only set one to the node in the inspector. It should allow to set repeat enable, to any texture that is in the shader, not only the one in the scene.

kleonc commented 1 year ago

I hope this demonstrates the problem correctly: image

Using SamplerPort as the source, it uses the texture from the scene. But I want to set repeat on the noise, which I have imported from the FileSystem.

@DasGandlaf You've not connected it to the output color, that's why the default texture is shown. After adding a Texture2DParameter node you should be able to assign a texture to it through the inspector (~be sure to save the shader after adding such node~ edit: it actually seems to show up in the inspector only if the texture is being used, like e.g. when it's connected to the output color): Godot_v4 0 2-stable_win64_Ca82d6ErdFFMfUl0f8wI

Note the texture I've set is seamless: obraz

DasGandlaf commented 1 year ago

@kleonc I'm afraid you don't understand the problem. I don't want to output the noise texture. Make a sprite with the godot icon, the godot icon is supposed to be shown. Now, add a shader. In that shader, add a scrolling noise texture. That shouldn't be possible, since you can only repeat, whatever is in the texture of the sprite2d Node.

But if you say to change the texture of the sprite2d node to a noise, and in the shader to set the color output to the desired texture, in this case the godot icon, the question arises, as to how one can make 2 scrolling noise textures, which one wants to combine, to get a desired effect.

kleonc commented 1 year ago

@kleonc I'm afraid you don't understand the problem. I don't want to output the noise texture. Make a sprite with the godot icon, the godot icon is supposed to be shown. Now, add a shader. In that shader, add a scrolling noise texture. That shouldn't be possible, since you can only repeat, whatever is in the texture of the sprite2d Node.

But if you say to change the texture of the sprite2d node to a noise, and in the shader to set the color output to the desired texture, in this case the godot icon, the question arises, as to how one can make 2 scrolling noise textures, which one wants to combine, to get a desired effect.

Something like this?

ipKefBLyuV Godot_v4 0 2-stable_win64_TJI238wbgW byx2NN5hys

DasGandlaf commented 1 year ago

Oh.. so SamplerPort is a shader parameter, that takes a texture. I thought it uses the texture from the sprite2d node. I did not understand that until now. Maybe I didn't know it because of my limited knowledge of shaders, but I think maybe it should be more intuitive? To somehow mark nodes that take shader parameters. I think some people may get confused about that. @kleonc @Chaosus Thank you both.

Zireael07 commented 1 year ago

Sampler port is the literal equivalent of sampler2D in shader code, how could it be more intuitive??

DasGandlaf commented 1 year ago

Nobody needs your stupid question. The reason why I and many other people are using visual shader is, guess what, because we don't know how to code shaders. Now if you think about it, that would mean, we don't know know what Samplerport means in shader code either (mind blown).

How could it be more intuitive? Well. Just add a little text at the top, (shader parameter). Do you see, how it can possibly be more intuitive? Either don't try to make me look stupid, or reformulate your sentence.

Zireael07 commented 1 year ago

All ports are shader parameters. So that little text is NOT helpful at all.

Things like this are exactly why I asked, because things that you think are helpful aren't necessarily helpful AND/OR as a coder I don't know what visual shader users expect.

Also: we have a code of conduct in this repo and calling others stupid does not jive with it.

aldocd4 commented 1 year ago

Hello, I had same issue, Texture2DParameter seems to fix the problem.

But I think it would help a lot if we had possibility to set a default value in Texture2DParameter node (like for other parameters nodes). It's hard to switch from fullscreen visual shader window to scene window each time we update something.

If we could see the result directly in the visual shader editor it would be cool.

Current setup: image

sdrib commented 1 year ago

I ran in this exact same issue and first found this thread: https://github.com/godotengine/godot/issues/74310

I think that is the same issue? Texture2DParameter is a decent enough workaround for now but I was trying to make it work using a texture in the editor (like the original report here).

Is that how it eventually will work? Or would we always have to create parameters to make 2D textures repeat 🤔 ?

sdrib commented 1 year ago

Learning more about visual shaders in Godot and there's a gap there in documentation I guess, though it is being discussed in blog post like this one https://godotengine.org/article/major-update-visual-shaders-godot-3-2-part-2/#sampler-type

That's 3.2 so it doesn't have the 4.x repeat option, but the 4.1 docs don't even mention sampler type (in meaningful ways) https://docs.godotengine.org/en/stable/search.html?q=sampler+type&check_keywords=yes&area=default

ducklin5 commented 9 months ago

I've also run into this issue and wasn't sure how to get the desired result in visual shader until I read this thread. I started with a design very similar to the first one: image

This doesn't do what it should do because I've set seamless as true but the change isn't reflected.

As for the workaround using Sampler2D, I somewhat agree that it's not very intuitive. Without this thread, I wouldn't know what that sampler2d input is for. It might be worth renaming or captioning that input to something like "External Texture".


Edit: Its also not very intuitive that the samper2d port has no effect at all unless you set "SamplerPort" for the drop down of 8 other options

eddyortegaa commented 4 months ago

Has anyone had any updates on this issue? I've tried everything and can't get a noise texture to pan along with time, seems like a pretty basic thing to do, am I missing something?