godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 96 forks source link

Separate Texture clamp/repeat/mirror flags into an enum for each axis #246

Open bates64 opened 4 years ago

bates64 commented 4 years ago

Context

I'm currently working on a Paper Mario 64 map importer. It involves dynamically creating ImageTextures from texture images, which I cannot change. The game uses space-saving measures very often, ie. having a texture be repeated and then mapping vertices in that repeated space. This includes textures which are clamped in one axis and mirrored in another, which Godot doesn't support! 😮

My scene rendered in Godot The original texture

For the particular texture above, yes, mirroring on both axes would be sufficient. However, my tool's dynamic nature cannot work that out; instead, I use parse texture config data like--

tex: kmr_wood_dtif
{
    img: kmr_wood_dtif.png
    {
        format: CI-4
        hwrap: repeat
        vwrap: mirror
    }
    filter: yes
    combine: 8
}

--which, you will note, sometimes has different values for hwrap and vwrap. Godot won't let me handle these values accurately as of right now.

The Texture class supports OpenGL's repeat/mirror/clamp in the 'flags' section, but doesn't let me specify modes for each axis - despite support in OpenGL, Godot combines the axes into three flags. I can't say I understand why such a decision was made, but there we go.

Proposal

@clayjohn suggests this enhancement is a candidate for 4.0. That means we can be a little more breaking:

I propose moving clamp/repeat/mirror away from the Texture.FLAG_XXX enum. Especially because, right now, you can enable both 'Repeat' and 'Mirrored Repeat' which results in the same behaviour as just enabling the latter. That shouldn't even be possible: they're mutually exclusive behaviours!

image

(Ticking neither of these boxes results in the WRAP_CLAMP mode described below.)

Instead, these flags can be replaced with a pair of new properties, called wrap_x (horizontal), wrap_y (vertical), wrap_z (depth), which each may hold one of the following values:

extends Texture

enum WrapMode {
  WRAP_CLAMP,
  WRAP_REPEAT,
  WRAP_MIRROR
}

var wrap_x: WrapMode = WrapMode.WRAP_CLAMP
var wrap_y: WrapMode = WrapMode.WRAP_CLAMP
var wrap_z: WrapMode = WrapMode.WRAP_CLAMP # `extends TextureLayered` only

These variants map cleanly to OpenGL's GL_TEXTURE_WRAP_S and GL_TEXTURE_WRAP_T texture parameters: GL_CLAMP_TO_EDGE, GL_REPEAT, GL_MIRRORED_REPEAT. The same variants are supported by Metal, and, more importantly for 4.0, Vulkan.

As a comparison, Unity does something very similar, but calls the properties wrapModeU, wrapModeV, and wrapModeW respectively. It also has a property for setting all three of these at once, which I argue doesn't need to be done in our implementation: it's incredibly easy to just set all three to the same value if desired, both in GDScript and in GUI.


This enhancement could be worked around with a bunch of user-written shader code which implements separate-axis mirroring/repeating/clamping, but that's definitely not an elegant, efficient solution. The engine supporting this directly wouldn't impede on any existing users, nor is it going to bloat download size.

Calinou commented 4 years ago

I can't say I understand why such a decision was made, but there we go.

It was likely done this way in the interest of simplicity. If nobody needed it before, there was no use in exposing it :slightly_smiling_face:

Also, while I agree with this proposal, adding separate flags for each axis could make it more difficult for newcomers to configure texture flags in the Import dock. If you want a flag to apply to all axes, you'll need to set it three times instead of just once.

Zireael07 commented 4 years ago

Maybe make a default fourth checkbox, 'All axes'?

bates64 commented 4 years ago

Would this fourth checkbox need to have a script equivalent? Might it be possible to do (pseudocode, I understand we actually implement this in C++!) the following?

var wrap_all setget _wrap_set_all _wrap_get_all

func _wrap_set_all(_mode):
    wrap_x = _mode
    wrap_y = _mode
    if self is TextureLayered: 
        wrap_z = _mode

func _wrap_get_all():
    if wrap_x == wrap_y:
        if not (self is TextureLayered) or wrap_x == wrap_z:
            return wrap_x
    else:
        return null

This does add, imo, unnecessary complexity. (It does prompt me to ask why the following syntax isn't supported in GDScript, though:

wrap_x = wrap_y = WRAP_REPEAT

)

Having three separate flags could make it more difficult for newcomers to configure texture flags in the Import dock

"Wrap mode X" and "Wrap mode Y" (the third axis option only appears on Texture3D!) enumerated options are, in my opinion, definitely not too complex, even for newcomers. Especially because the result can be seen in the preview window, it should be fairly intuitive. Maybe I'm wrong on that.

OhiraKyou commented 1 year ago

Separate axes are pretty important for my workflow. I often use color lookup textures that must wrap horizontally and clamp vertically.

Calinou commented 1 year ago

Separate axes are pretty important for my workflow. I often use color lookup textures that must wrap horizontally and clamp vertically.

Can you post an example LUT that requires this (along with the texture you're using the LUT on)? This would help better understand why it's needed.

OhiraKyou commented 1 year ago

There isn't a separate LUT and texture; the texture is the lookup, and it acts as a per-material color palette. The technique I developed uses a texture whose x-axis represents hue and y-axis represents a value I refer to as "exposure". The shader, then, procedurally calculates "spectral coordinates". This ensures that colors remain "on-spectrum". Hue should repeat, but exposure should be clamped. In Unity, I do this in the texture import settings.

This is similar to the gradient maps used by artists like Joyce(MinionsArt), but more procedural.

Basic RGB example spectrum:

OhiraKyou - Spectrum Example 1-1

When this spectrum is wrapped horizontally, hue is correctly shifted (in theory, after PR #79685 hits stable). However, wrapping vertically results in black spots on highlights and/or white spots on shadows.

Here's an in-game example, with dark highlights on bright rocks caused by wrapping from the bright to dark side:

OhiraKyou - Spectral Wrapping - Example 1-1 (dark highlights)

It's important to keep in mind that textures are just data stores that can be read by shaders, and developers use them to store a wide variety of map types. Stylized games, in particular, can require custom data workflows. And, sometimes, that data needs to repeat in one axis and be clamped in another.

mariomadproductions commented 11 months ago

Is there a good workaround for this issue?

OhiraKyou commented 11 months ago

@mariomadproductions

Is there a good workaround for this issue?

For now, my workaround is to use repeat, but manually clamp one UV axis (in-shader) between the centers of its first and last pixel. However, I also don't apply filtering or compression, because I'm sampling from what is, effectively, a color palette. If you use any import settings or processing techniques that introduce sampling fuzziness, you may still observe artifacts from pixel interpolation across the image bounds.

My shader function:

// Clamps the y axis, thereby working around the lack of separate axis repeat settings
vec4 sample_texture_repeat_x_clamp_y(sampler2D input_texture, vec2 base_uv) {
    ivec2 input_texture_size = textureSize(input_texture, 0);

    // Allow x to repeat
    float x = base_uv.x;

    // Clamp y between the middle of the first and last pixel
    float texture_height_pixels = float(input_texture_size.y);
    float half_pixel_fraction = 0.5 / texture_height_pixels;
    float y = clamp(base_uv.y, half_pixel_fraction, 1.0 - half_pixel_fraction);

    // Sample the texture
    vec2 modified_uv = vec2(x, y);
    vec4 sampled_color = texture(input_texture, modified_uv);

    return sampled_color;
}
Calinou commented 11 months ago

Now that 4.0 is released and BaseMaterial3D uses a boolean texture_repeat property, we should find a way to expose this without breaking compatibility. Perhaps this would need to be a texture_repeat_axes enum property that defaults to X/Y/Z, but can be set to any of the following:

CanvasItem uses an enum for its texture_repeat property, but it has disabled/enabled/mirror options. This means that if we use the same approach, you wouldn't be able to use enabled repeat and mirror repeat at the same time on different axes, which is a limitation we have to account for with this proposed API.

Regarding the technical implementation, Godot keeps several samplers around that have filter and repeat disabled/enabled: https://github.com/godotengine/godot/blob/09946f79bd8215b2c6332de8821737580909a91c/servers/rendering/renderer_rd/storage_rd/material_storage.cpp#L1098-L1109

We will need to find a way to achieve this without multiplying the number of samplers by several times.