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

Clarify in default texture repeat and filter docs that they only impact the built in texture #98738

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 1, 2024

Helps clarify for situations like #57679 where users expect that this setting will apply to all textures.

@clayjohn clayjohn requested a review from a team as a code owner November 1, 2024 21:41
@clayjohn clayjohn added this to the 4.4 milestone Nov 1, 2024
@clayjohn clayjohn added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Nov 1, 2024
@clayjohn clayjohn force-pushed the DOC-default-texture-filter branch from 58cbf09 to fa4f701 Compare November 5, 2024 20:14
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

The shader remark is out of place in my opinion. If it has to be somewhere it would be better off in CanvasItem itself.

@@ -2890,11 +2890,11 @@
If [code]true[/code], forces vertex shading for all rendering. This can increase performance a lot, but also reduces quality immensely. Can be used to optimize performance on low-end mobile devices.
</member>
<member name="rendering/textures/canvas_textures/default_texture_filter" type="int" setter="" getter="" default="1">
The default texture filtering mode to use on [CanvasItem]s.
The default texture filtering mode to use for [CanvasItem]s built-in texture. In shaders, this texture is accessed as [code]TEXTURE[/code].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The default texture filtering mode to use for [CanvasItem]s built-in texture. In shaders, this texture is accessed as [code]TEXTURE[/code].
The default texture filtering mode used to render the built-in texture for [CanvasItem] nodes. In shaders, this texture is accessed as [code]TEXTURE[/code].

Copy link
Contributor

Choose a reason for hiding this comment

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

Notably texture is not a property of CanvasItem - it's a property of several nodes that inherit CanvasItem:
https://docs.godotengine.org/en/stable/classes/class_texturerect.html#class-texturerect-property-texture
https://docs.godotengine.org/en/stable/classes/class_sprite2d.html#class-sprite2d-property-texture
https://docs.godotengine.org/en/stable/classes/class_line2d.html#class-line2d-property-texture
If this note goes on texture, it would have to be repeated in several places.

I also think that the project settings are a reasonable place to put general information about Godot that don't fit anywhere else in the class reference. At least that's how they are used currently. This note seems fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

Also seems fine to me. :)

[b]Note:[/b] For pixel art aesthetics, see also [member rendering/2d/snap/snap_2d_vertices_to_pixel] and [member rendering/2d/snap/snap_2d_transforms_to_pixel].
</member>
<member name="rendering/textures/canvas_textures/default_texture_repeat" type="int" setter="" getter="" default="0">
The default texture repeating mode to use on [CanvasItem]s.
The default texture repeating mode to use for [CanvasItem]s built-in texture. In shaders, this texture is accessed as [code]TEXTURE[/code].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The default texture repeating mode to use for [CanvasItem]s built-in texture. In shaders, this texture is accessed as [code]TEXTURE[/code].
The default texture repeating mode used to render the built-in texture for [CanvasItem] nodes. In shaders, this texture is accessed as [code]TEXTURE[/code].

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this addition is correct. Or, at the very least, it is confusing.

This setting changes how the texture is sampled when it is used in a shader. In some situations that may change how the CanvasItem is rendered to the Viewport, and in others it won't. Importantly though, this setting doesn't change how the texture itself is created, which this addition seems to say.

Sampling is the process of reading from a texture. So basically this tells the shader what to do when it is asked to read from an area outside the texture's boundary.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

I think this is fine as is.

@Repiteo Repiteo merged commit 4e7cf69 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release documentation enhancement topic:shaders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants