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

Replace voxel uvmap interface with uv_transform interface #4758

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 27, 2025

Description

The current interface only allows you to define a LRBT for the texture coordinates of a voxel. This means that you can't rotate the textures applied to voxels. The uv_transform interface allows for this and also aims to solve more or less the same problem for meshscatter. So seems natural to me to reuse it here.

Quick example:

using FileIO, LinearAlgebra
cow = FileIO.load(assetpath("cow.png"))
voxels(ones(UInt8, 1,1,1), color = cow; uv_transform = [:rotr90])

image

Type of change

  • somewhat breaking refactor:
    • soft-deprecated uvmap, i.e. automatic converison to uv_transform with warning
    • changed top and bottom uv layout to align with x, y direction

Checklist

  • fix :rotr/:rotl being switched in uv_transform
  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 27, 2025

Benchmark Results

SHA: 53987012ad8bfdd38f77ee0af4dffd12279b84b5

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@enbyd
Copy link

enbyd commented Jan 27, 2025

~$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.11.0-rc3 (2024-08-26)
 _/ |\__'_|_|_|\__'_|  |  Fedora 40 build
|__/                   |

julia> ]add WGLMakie#47de4854c97ea14c924b92af14e9dbd52b2ae85c

julia> using WGLMakie, FileIO

julia> uvtest = load(<Path to sample texture>);

julia> typeof(uvtest)
Matrix{RGBA{N0f8}} (alias for Array{ColorTypes.RGBA{FixedPointNumbers.Normed{UInt8, 8}}, 2})

julia> voxels(ones(UInt8, 1,1,1), color = uvtest)
KeyError: key :uv_transform not found

julia> voxels(ones(UInt8, 1,1,1), color = uvtest; uv_transform = [:rotr90])
ERROR: Invalid attribute uv_transform for plot type Voxels{Tuple{MakieCore.EndPoints{Float32}, MakieCore.EndPoints{Float32}, MakieCore.EndPoints{Float32}, Array{UInt8, 3}}}.

The available plot attributes for Voxels{Tuple{MakieCore.EndPoints{Float32}, MakieCore.EndPoints{Float32}, MakieCore.EndPoints{Float32}, Array{UInt8, 3}}} are:

alpha        color       colorscale    diffuse  highclip         inspector_hover  is_air    model      shading    specular        transparency          
backlight    colormap    depth_shift   fxaa     inspectable      inspector_label  lowclip   nan_color  shininess  ssao            uvmap                 
clip_planes  colorrange  depthsorting  gap      inspector_clear  interpolate      material  overdraw   space      transformation  visible       

uvmap is listed in the available plot attributes, uv_transform is not

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 27, 2025

@enbyd you would have to add all Makie packages in the stack:

add MakieCore#47de4854c97ea14c924b92af14e9dbd52b2ae85c Makie#47de4854c97ea14c924b92af14e9dbd52b2ae85c WGLMakie#47de4854c97ea14c924b92af14e9dbd52b2ae85c

@enbyd
Copy link

enbyd commented Jan 27, 2025

@asinghvi17 thank you! This resolved the earlier issue and i can now replicate the example given in the pr, i.e. when passing a value for the uv_transform attribute (such as :rotr90), the full texture is displayed as expected, with the desired transformation.

However when the attribute is not passed, as such:
julia> voxels(ones(UInt8, 1,1,1), color = uvtest)
it seems like just the first pixel in the texture is mapped to the faces, rather than the full texture. Is the intent for the automatic behavior not to use the full image?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 27, 2025

However when the attribute is not passed, as such: julia> voxels(ones(UInt8, 1,1,1), color = uvtest) it seems like just the first pixel in the texture is mapped to the faces, rather than the full texture. Is the intent for the automatic behavior not to use the full image?

That's undefined behavior. It was before this pr too. But it would be good if this either errors or does something reasonable. I'm leaning towards making this an error, because it seems more likely to me that someone would forget to pass a uv_transform than skipping it on purpose. But I could very well be blind to what users actually want here.

In general - if you have (more) complaints/feedback on voxels I'd love to hear it. I think that would be quite helpful to polish voxel plots.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 28, 2025

I noticed that gap > 0 does not scale uv coordinates, but instead just cut them off. Should that be changed?

tex = FileIO.load(assetpath("cow.png"));
f,a,p = voxels(ones(UInt8, 2, 2, 2), color = tex; uv_transform = [I])
voxels(f[1,2], ones(UInt8, 2, 2, 2), color = tex; uv_transform = [I], gap = 0.5)
f

image

@enbyd
Copy link

enbyd commented Jan 28, 2025

I noticed that gap > 0 does not scale uv coordinates, but instead just cut them off. Should that be changed?

Maybe. Though it is a bit of a strange case since it behaves like padding voxels within some bounded region, which is less clear cut than other examples.
Imagining some hypothetical attributes for a moment, to compare:
If there were a spacing attribute that kept everything the same scale and just moved voxels farther apart, I would definitely expect the texture mapping to stay the same.
If there was a scale attribute that just changed the scale factor of the whole plotted object, I would again expect that the texture map would be scaled as well, and preserve appearance on the voxel(s).
Same even if the transformation was more generalized (as in: per-axis-scaling, -rotation, and -translation). I'd expect that to take place after the texture was mapped, so that the texture didn't have to have the same transformation(s) applied to it independently, and then from there onto the voxel(s) (at least from an end user POV)

But gap is less clear for me.
Without any texture data, it's indistinguishable from a spacing+scaling+translation, so maybe it should also preserve textures by extension? I would personally have more use for each of those imagined "component" transformations than I do for gap though, so maybe without a use case I have a hard time deciding.

For context, most of my experience mapping textures to voxels is using Mojang's resource pack specification for Minecraft, which supports custom block models made from sets of generalized cuboids, to which rectangular texture regions can be mapped. And there, any transformation applied to the cuboid takes the texture with it; you do not need to reconfigure the UVs. Have you used that system before?

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 28, 2025

I would agree that gap should scale the UVs to preserve textures - it makes life a lot simpler. And people can always change the uv_transform if they don't want that.

@enbyd
Copy link

enbyd commented Jan 28, 2025

That's undefined behavior. It was before this pr too. But it would be good if this either errors or does something reasonable. I'm leaning towards making this an error, because it seems more likely to me that someone would forget to pass a uv_transform than skipping it on purpose. But I could very well be blind to what users actually want here.

I spoke with intricate yesterday to try and formulate a clear response. We also generally agreed that it would be more likely that someone forgets the attribute than that they want to intentionally leave it out, assuming they are applying a texture. If someone is using the color attribute for a color only (not a texture) I would not expect them to use uv_transform. Maybe a separate attribute for applying a texture is called for, to disambiguate those cases.

Also, if when applying a texture you must include uv_transform, I think there should be a string option for "no transformation, just apply the texture". The set feels incomplete if it has all the basic rotations and mirroring but not the identity. (Please let me know if this exists and I missed it)

@asinghvi17
Copy link
Member

I believe uv_transform=:identity is a thing.

@enbyd
Copy link

enbyd commented Jan 28, 2025

I believe uv_transform=:identity is a thing.

That would be great, but is there a list of these somewhere? I looked through the code for similar strings and found some, but the only set that I found that could return an identity matrix was commented out and was called :surface (i think for Image uv mapping? I forget) I was unable to find the code that parses uv_transform for voxels. Not familiar enough with the call stack yet to trace it easily

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 28, 2025

They are constructed by the uv_transform constructor, and instantiated in the convert_attributes pipeline.

Actually you're right that there's no :identity, we should probably add that. But you can pass the identity matrix (I from LinearAlgebra) and that works fine, which should probably be documented better.

  uv_transform(action::Symbol)

  Creates a 3x3 uv transformation matrix from a given named action. They assume 0 < uv < 1 and thus may not work correctly with Patterns. The actions include

    •  :rotr90 corresponding to rotr90(texture)

    •  :rotl90 corresponding to rotl90(texture)

    •  :rot180 corresponding to rot180(texture)

    •  :swap_xy, :transpose which corresponds to transposing the texture

    •  :flip_x, :flip_y, :flip_xy which flips the x/y/both axis of a texture

    •  :mesh, :meshscatter, :surface, :image which grabs the default of the corresponding plot type

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 28, 2025

On gap - I also think that scaling would be better. So I guess I'll change this here too.

W.r.t per voxel (?) transformations - that's impossible with the way they are rendered here. The shader is just drawing a bunch of large planes that combine into many voxels, rather than cubes. So any transformation has to be global. gap moves the front and back planes (etc) closer together to effectively shrink all voxels.

If someone is using the color attribute for a color only (not a texture) I would not expect them to use uv_transform.

Right, there are ~three modes atm: A color Vector that acts as a map from voxel id -> color, colormaps that basically do the same thing but with interpolate colors instead of doing a direct id -> color mapping and image colors that do textures mapping. Only the last case complains if not uv_transform is given. And inversely, a uv_transform with an color image also complains now.

I believe uv_transform=:identity is a thing.

That does exist as uv_transform = [I] for voxels. But yea, I don't think it's documented in ?uv_transform. I should probably move some code around so it makes sense to document there.

TODO I'm taking away from this:

  • make uv's scale with gap so they keep fully covering the shrunk voxel
  • improve docs on uv_transform

@enbyd
Copy link

enbyd commented Jan 28, 2025

W.r.t per voxel (?) transformations - that's impossible with the way they are rendered here. The shader is just drawing a bunch of large planes that combine into many voxels, rather than cubes. So any transformation has to be global. gap moves the front and back planes (etc) closer together to effectively shrink all voxels.

Understood, thanks. For generalized cuboids like those I mentioned, would you recommend mesh_scatter? Or something else?

Also, out of curiosity, is there support or plans for backface culling (or visibility culling in general, for that matter)? The rendering is already relatively performant, but I'm wondering how much more time you'd wager there is to reasonably squeeze out of the renderer, and face culling comes to mind as one possible way.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 29, 2025

With gap = 0 the rendering doesn't generate any backfaces. And I'm not sure how much visibility culling would actually do, because you'd need to verify a whole slice of the chunk before you could discard the ones behind.
If you're interested the current implementation is based on https://www.youtube.com/watch?v=4xs66m1Of4A. It's pretty simple and effective in terms of doing lots of voxels. As far as I can tell it's entirely memory bound, so the thing to optimize would be the 3D texture. I.e. stuff like brickmaps, sparse voxel octrees etc. I played around a bit with those but didn't really understand how or if that would be a general improvement to a UInt8 texture. I think in terms of rendering algorithm the next step up would probably be a ray traced solution.

Understood, thanks. For generalized cuboids like those I mentioned, would you recommend mesh_scatter? Or something else?

If you're looking to load a voxel model and transform the whole thing as one you can still do that with a voxel plot. I.e. using translate!/rotate!/scale!(plot, ...). Otherwise, yea, meshscatter with a Rect3f marker is probably best

@enbyd
Copy link

enbyd commented Jan 30, 2025

I noticed that the uv debug image was intentionally rotated so that when using that debug image, the texture appears upright using the current default mapping. @lntricate1 and I have been discussing default texture mapping over the past couple days to come up with what we consider a sensible alternative to the current default mapping that will work better for how voxel texture packs tend to be designed and used.

These pictures show how we'd expect an orientable texture to appear on a voxel.
image1
image2

To summarize, the side textures appear upright (and the texture itself looks exactly like that in any editor or viewer), the top face appears upright when looking towards -x, and the bottom face also appears upright when looking towards -x.

To render this, we used these transformations:
uv_transform = reshape([:rotr90, :rotr90, :flip_x, :rotr90, :rotr90, I], (1,6))
The proposal is for the new default mapping to look this way.

The hope is that this will better match user expectations by default, while also still permitting general transformations as needed.

@enbyd
Copy link

enbyd commented Jan 30, 2025

If you're interested the current implementation is based on https://www.youtube.com/watch?v=4xs66m1Of4A.

This was an interesting rewatch. I'd seen it closer to when it was published and thought it was a clever approach, so it's cool to see it implemented here. One of the common comments on the page there is about overdraw, and how there are whole planes that may be mostly or entirely hidden from view. I think my initial comment about culling was prompted by similar observations, in particular by flying through a mass of voxels, and seeing all of the middles of the planes visible there. But yeah it sounds like the tradeoff between this method and another that uses something like raymarching at least depends on world size.

I did also start poking around for info and research on brickmaps and SVOs. I was able to find more info about the latter. Both sounded neat. Thanks for the info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

4 participants