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

[BUG] fix rounding issue in uv_to_color() #2336

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

TooSchoolForCool
Copy link
Contributor

Hi Mike,

I discovered an index out of bounds error in the to_color() function when converting TextureVisual to ColorVisuals.

The issue occurs in the uv_to_color() function here:

# get texture image pixel positions of UV coordinates
x = (uv[:, 0] * (image.width - 1)) % image.width
y = ((1 - uv[:, 1]) * (image.height - 1)) % image.height

# access colors from pixel locations
# make sure image is RGBA before getting values
colors = np.asanyarray(image.convert("RGBA"))[
    y.round().astype(np.int64), x.round().astype(np.int64)
]

The modulus (%) operation on float values can cause index out of bounds errors when rounding values near the bounds. For example:

  1. Given uv[i, 0] == -26.05739974975586 and image.width == 456
  2. Results in x[i] == 455.883113861084 after modulus operation x = (uv[:, 0] * (image.width - 1)) % image.width
  3. When rounded, x.round().astype(np.int64) == 456
  4. This exceeds valid image indices [0, 455], causing the error

I have fixed this issue in this pull request, please let me know if you have any further concerns!

Best,

Zeyu

@mikedh mikedh changed the base branch from main to refactor/loadtype December 30, 2024 02:47
@mikedh mikedh merged commit c46eca1 into mikedh:refactor/loadtype Jan 2, 2025
7 of 9 checks passed
@mikedh
Copy link
Owner

mikedh commented Jan 2, 2025

Thanks for the fix!! I suppose there are multiple ways of handling overflowing UV's like wrap vs clamp vs mirror and this selects "wrap" as the default here. This makes sense to me, and is a lot better than crashing!

mikedh added a commit that referenced this pull request Jan 21, 2025
A very common source of annoyance and confusion is that `trimesh.load`
can return lots of different types depending on what type of file was
passed (i.e. #2239). This refactor changes the return types for the
loading functions to:

- `trimesh.load_scene -> Scene`
- This loads into a `Scene`, the most general container which can hold
any loadable type. Most people should probably use this to load
geometry.
- `trimesh.load_mesh -> Trimesh`
- Forces all mesh objects in a scene into a single `Trimesh` object.
This potentially has to drop information and irreversibly concatenate
multiple meshes.
- The implementation of the concatenation logic is now in
`Scene.to_mesh` rather than load.
- `trimesh.load_path -> Path`
- This loads into either a `Path2D` or `Path3D` which both inherit from
`Path`
- `trimesh.load -> Geometry`
- This was the original load entry point and is deprecated, but there
are no current plans to remove it. It has been modified into a thin
wrapper for `load_scene` that attempts to match the behavior of the
previous loader for backwards compatibility. In my testing against the
current `main` branch it was returning the same types [99.8% of the
time](https://gist.github.com/mikedh/8de541e066ce842932b1f6cd97c214ca)
although there may be other subtle differences.
- `trimesh.load(..., force='mesh')` will emit a deprecation warning in
favor of `load_mesh`
- `trimesh.load(..., force='scene')` will emit a deprecation warning in
favor of `load_scene`

Additional changes:
- Removes `Geometry.metadata['file_path']` in favor of
`Geometry.source.file_path`. Everything that inherits from `Geometry`
should now have a `.source` attribute which is a typed dataclass. This
was something of a struggle as `file_path` was populated into metadata
on load, but we also try to make sure `metadata` is preserved through
round-trips if at all possible. And so the `load` inserted *different*
keys into the metadata. Making it first-class information rather than a
loose key seems like an improvement, but users will have to replace
`mesh.metadata["file_name"]` with `mesh.source.file_name`.
- Moves all network fetching into `WebResolver` so it can be more easily
gated by `allow_remote`.
- Removes code for the following deprecations:
- January 2025 deprecation for `trimesh.resources.get` in favor of the
typed alternatives (`get_json`, `get_bytes`, etc).
- January 2025 deprecation for `Scene.deduplicated` in favor of a very
short list comprehension on `Scene.duplicate_nodes`
- March 2024 deprecation for `trimesh.graph.smoothed` in favor of
`trimesh.graph.smooth_shaded`.
- Adds the following new deprecations:
- January 2026 `Path3D.to_planar` -> `Path3D.to_2D` to be consistent
with `Path2D.to_3D`.
- Fixes #2335 
- Fixes #2330 
- Fixes #2239
- Releases #2313 
- Releases #2327 
- Releases #2336
- Releases #2339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants