Conversation
📝 WalkthroughWalkthroughThis pull request adds mipmapping support: an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TextureInit as Texture.__init__
participant MipGen as Mipmap Generation
participant TexCreate as _create_texture
participant CUDA as CUDA Driver
Client->>TextureInit: create(texture, num_mip_levels > 1, mip_filter_mode)
TextureInit->>MipGen: compute/generate mip chain (_compute_mip_count/_generate_mip_chain)
MipGen-->>TextureInit: concatenated_data, mip_widths, mip_heights, (mip_depths)
TextureInit->>TexCreate: _create_texture(data, mip_widths, mip_heights, mip_depths)
TexCreate->>CUDA: cuMipmappedArrayCreate + cuMipmappedArrayGetLevel (copy per-level)
CUDA-->>TexCreate: mipmap_handle, level arrays populated
TexCreate-->>TextureInit: texture_handle, mipmap_handle
TextureInit-->>Client: Texture object ready (with mipmap state)
sequenceDiagram
participant Caller
participant texture_sample
participant Helper as texture_sample_helper
participant Backend as Backend (CUDA/CPU)
Caller->>texture_sample: sample(tex, coords..., lod)
texture_sample->>Helper: dispatch specialization (float/vec2/vec4)
alt lod > 0 (explicit level)
Helper->>Backend: tex2DLoad/tex3DLoad or cpu_sample_*_mip (use explicit level)
Backend-->>Helper: texel from specified mip level
else lod == 0 (default)
Helper->>Backend: tex2D/tex3D or cpu_sample_* (standard filtering across mips)
Backend-->>Helper: filtered texel
end
Helper-->>texture_sample: sampled value
texture_sample-->>Caller: return value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR adds mipmap support to Warp's texture API, enabling automatic generation of mip chains and LOD-based sampling for both 2D and 3D textures. The implementation includes Python-side mipmap generation using box filter downsampling, CUDA mipmap array creation, and CPU software mipmap sampling to match CUDA behavior. Key Changes
Critical Issues FoundThe CPU texture sampling implementation has bugs that will cause incorrect behavior:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Texture2D/3D
participant Python texture.py
participant Native texture.cpp
participant CUDA/CPU
User->>Texture2D/3D: __init__(data, num_mip_levels=0)
Texture2D/3D->>Python texture.py: _compute_mip_count()
Python texture.py-->>Texture2D/3D: calculated levels
Texture2D/3D->>Python texture.py: _generate_mip_chain_2d/3d()
Python texture.py->>Python texture.py: _downsample_axis() repeatedly
Python texture.py-->>Texture2D/3D: concatenated mip data
Texture2D/3D->>Native texture.cpp: wp_texture2d_create_device()
Native texture.cpp->>CUDA/CPU: create texture with mipmap array
CUDA/CPU-->>Native texture.cpp: texture handle
Native texture.cpp-->>Texture2D/3D: texture created
User->>User: wp.launch(kernel)
User->>CUDA/CPU: texture_sample(tex, uv, dtype, lod)
alt CUDA Path
CUDA/CPU->>CUDA/CPU: tex2DLod/tex3DLod (hardware)
else CPU Path
CUDA/CPU->>CUDA/CPU: cpu_sample_2d_channel_mip()
CUDA/CPU->>CUDA/CPU: interpolate between mip levels
end
CUDA/CPU-->>User: sampled value
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -258,7 +309,7 @@ inline float cpu_sample_2d_channel(const cpu_texture2d_data* tex, float u, float | |||
| y = cpu_clamp_index(y, tex->height); | |||
| } | |||
There was a problem hiding this comment.
Mip coords use base dims
When normalized_coords=False, cpu_sample_2d_channel() (and the 3D variant) converts texel-space u/v(/w) to normalized coords using tex->width/height/depth even when sampling a mip level, and the 2D nearest path clamps x/y using tex->width/height as well. For level>0 this will sample the wrong location (and can change border/clamp behavior) because the selected mip has different dimensions. Use the local width/height(/depth) variables for the unnormalized conversion and clamping when level>=0.
Also appears in: warp/native/texture.h lines 354-357 (3D conversion), 305-310 (2D clamping).
| if (tex.tex == 0) | ||
| return vec2f(0.0f, 0.0f); | ||
| const cpu_texture2d_data* cpu_tex = (const cpu_texture2d_data*)tex.tex; | ||
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0), cpu_sample_2d_channel(cpu_tex, u, v, 1)); | ||
| if (load <= 0.0f) { | ||
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, 0), cpu_sample_2d_channel(cpu_tex, u, v, 1, 0)); | ||
| } else { | ||
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, load), cpu_sample_2d_channel(cpu_tex, u, v, 1, load)); | ||
| } |
There was a problem hiding this comment.
CPU vecN mip sampling wrong
In the CPU path for vec2f/vec4f, the mip branch calls cpu_sample_*_channel(..., load) but that last parameter is an int level, so load is implicitly truncated and fractional LOD interpolation + mip_filter_mode are bypassed. This makes CPU results disagree with CUDA and with the intended semantics for non-integer loads. These call sites should use cpu_sample_*_channel_mip(..., load) (or equivalent level0/level1/frac logic) for each channel.
Also appears in: warp/native/texture.h around lines 599-602 (vec4f 2D), 546-547 (vec2f 2D), 627-630 (vec4f 3D), and the corresponding vec2f 3D helper.
| # texture_sample for 2D textures with vec2 coordinates | ||
| add_builtin( | ||
| "texture_sample", | ||
| input_types={"tex": Texture2D, "uv": vec2f, "dtype": Any}, | ||
| input_types={"tex": Texture2D, "uv": vec2f, "dtype": Any, "load": float}, | ||
| defaults={"load": 0.0}, | ||
| value_func=texture_sample_2d_value_func, | ||
| export_func=lambda input_types: {k: v for k, v in input_types.items() if k != "dtype"}, | ||
| dispatch_func=texture_sample_2d_dispatch_func, |
There was a problem hiding this comment.
Stubs missing load
This PR adds load as an optional argument to texture_sample, but the generated type stubs (warp/__init__.pyi) still only expose the pre-existing overloads without load. That mismatch will cause IDE/typechecker errors for valid calls like wp.texture_sample(..., load=lod). Regenerate/update the stubs to include load: float = 0.0 for all texture_sample overloads (2D/3D, vec and scalar forms).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
warp/native/texture.h (1)
300-312:⚠️ Potential issue | 🔴 CriticalBug: 2D nearest-neighbor clamp uses base-level dimensions instead of mip-level dimensions.
Lines 306 and 309 clamp to
tex->width/tex->height(base-level dimensions), but when sampling a mip level the local variableswidth/height(set on lines 286–291) hold the correct mip-level dimensions. The 3D counterpart at lines 370–376 already uses the local variables correctly.This causes incorrect clamping and potential out-of-bounds reads when sampling mip levels with
FILTER_CLOSESTon 2D textures, since mip levels are always smaller than the base.🐛 Fix: use local width/height for clamping
if (tex->address_mode_u != WP_TEXTURE_ADDRESS_BORDER) { - x = cpu_clamp_index(x, tex->width); + x = cpu_clamp_index(x, width); } if (tex->address_mode_v != WP_TEXTURE_ADDRESS_BORDER) { - y = cpu_clamp_index(y, tex->height); + y = cpu_clamp_index(y, height); }warp/_src/builtins.py (1)
7619-7635:⚠️ Potential issue | 🟡 MinorRemove default-value wording for
loadand avoid “no mipmaps” phrasing.The docstring repeats the default and implies mipmaps are disabled; better to keep it semantic-only.
Also, since this file changed, regenerate the stubs withuv run build_docs.py.🧹 Proposed doc tweak
- load: Level of detail for mipmapped textures. Default is 0.0 (no mipmaps). + load: Level of detail for mipmapped textures.Run:
uv run build_docs.pyAs per coding guidelines, "Don't repeat default values from function signatures in docstrings—Sphinx autodoc shows them automatically" and "warp/_src/builtins.py: After modifying warp/_src/builtins.py, run build_docs.py to regenerate warp/init.pyi".
🤖 Fix all issues with AI agents
In `@warp/native/texture.cpp`:
- Around line 144-170: The three mallocs that create tex_data->mip_data,
tex_data->mip_widths and tex_data->mip_heights in the mipmapped 2D path (and the
analogous three mallocs in the 3D mipmapped path) must be null-checked: after
each malloc, verify the returned pointer is non-null; on any failure free any
prior successful allocations (free tex_data->mip_data, tex_data->mip_widths,
tex_data->mip_heights if set), free all_data, and return false (also ensure you
do not dereference null pointers afterward); apply the same pattern for the 3D
mipmap allocation block so both mipmapped paths safely clean up and return false
on allocation failure.
In `@warp/native/texture.h`:
- Around line 484-523: The CPU sampling branches in sample_2d and sample_3d call
cpu_sample_2d_channel / cpu_sample_3d_channel (and the vec2f/vec4f variants)
with level=0 which dereferences null mip arrays for non-mip textures; change
those non-mip calls to pass level = -1 (the default path that uses
tex->data/width/height) instead of 0 for all cpu_sample_*_channel(...) calls in
the load <= 0.0 branches across float, vec2f and vec4f specializations so
non-mipmapped textures no longer dereference mip pointers.
In `@warp/tests/cuda/test_texture.py`:
- Around line 2204-2209: The test_mipmap2d_max_load function currently defines
unused variables width and height while hardcoding the reshape to (4, 4); remove
the unused assignments or use them in the data.reshape call to fix the Ruff F841
warning. Locate the function test_mipmap2d_max_load and either delete the lines
"width, height = 4, 4" or change "data = np.arange(16,
dtype=np.float32).reshape((4, 4))" to use the width and height variables (e.g.,
reshape((height, width))) so the variables are referenced.
🧹 Nitpick comments (4)
warp/_src/texture.py (2)
594-605:_downsample_axissilently drops the last texel for odd-sized dimensions.When
nis odd (e.g., 5),new_n * 2 = 4, so the slice only covers indices 0–3 and index 4 is silently discarded. This gives correct results for power-of-2 dimensions, but for non-power-of-2 textures the mip chain loses edge data without warning.Since CUDA mipmapped arrays typically require power-of-2 dimensions, consider adding an early validation in
__init__(whennum_mip_levels > 1) that the base dimensions are powers of two, or at minimum document this limitation.
607-653: Unusednum_channelsparameter.Static analysis correctly flags that
num_channelsis unused in both_generate_mip_chain_2d(line 609) and_generate_mip_chain_3d(line 657). The channel dimension is handled implicitly by NumPy array shapes. Consider removing the parameter or prefixing with_if kept for future use.♻️ Suggested fix
`@staticmethod` def _generate_mip_chain_2d( - np_data: np.ndarray, num_levels: int, num_channels: int + np_data: np.ndarray, num_levels: int ) -> tuple[np.ndarray, np.ndarray, np.ndarray]:And update call sites accordingly (lines 283–284, 288–289).
warp/tests/cuda/test_texture.py (2)
2275-2309: Consider testing higher LOD levels for non-power-of-two textures.This test only samples at LOD 0. Non-power-of-two downscaling (e.g., 30→15→7→3→1) is the most error-prone path for mipmap generation. Sampling at higher LOD levels (especially the 1×1 level) would strengthen coverage.
2556-2567: Mipmap test coverage gaps to consider.A few scenarios aren't covered by the current suite:
- 3D mipmap with non-constant data —
test_mipmap3d_constant_valueonly validates that a uniform texture stays uniform across LOD levels. A gradient or pattern-based 3D test would catch downsampling bugs.- Fractional LOD — no test exercises fractional LOD values (e.g., 0.5) to validate trilinear interpolation between mip levels.
These could be added as follow-up.
| if (num_mip_levels > 1 && mip_widths != nullptr && mip_heights != nullptr) { | ||
| // Mipmapped path: compute total data size across all levels | ||
| size_t total_size = 0; | ||
| for (int i = 0; i < num_mip_levels; ++i) { | ||
| total_size += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | ||
| } | ||
|
|
||
| void* all_data = malloc(total_size); | ||
| if (all_data == nullptr) { | ||
| free(tex_data); | ||
| return false; | ||
| } | ||
| memcpy(all_data, data, total_size); | ||
|
|
||
| // Allocate mipmap arrays | ||
| tex_data->mip_data = (void**)malloc(sizeof(void*) * num_mip_levels); | ||
| tex_data->mip_widths = (int*)malloc(sizeof(int) * num_mip_levels); | ||
| tex_data->mip_heights = (int*)malloc(sizeof(int) * num_mip_levels); | ||
|
|
||
| // Set up per-level pointers into the contiguous buffer | ||
| char* ptr = (char*)all_data; | ||
| for (int i = 0; i < num_mip_levels; ++i) { | ||
| tex_data->mip_data[i] = ptr; | ||
| tex_data->mip_widths[i] = mip_widths[i]; | ||
| tex_data->mip_heights[i] = mip_heights[i]; | ||
| ptr += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | ||
| } |
There was a problem hiding this comment.
Missing null checks for mipmap sub-allocations.
The three malloc calls at lines 159–161 (and similarly at lines 282–285 for 3D) don't check for null returns. If any allocation fails after all_data succeeds, the code proceeds to dereference null pointers. While small-allocation malloc failures are rare, the non-mip path already validates tex_data->data.
🛡️ Example guard (2D path, lines 159-161)
tex_data->mip_data = (void**)malloc(sizeof(void*) * num_mip_levels);
tex_data->mip_widths = (int*)malloc(sizeof(int) * num_mip_levels);
tex_data->mip_heights = (int*)malloc(sizeof(int) * num_mip_levels);
+ if (tex_data->mip_data == nullptr || tex_data->mip_widths == nullptr || tex_data->mip_heights == nullptr) {
+ free(all_data);
+ free(tex_data->mip_data);
+ free(tex_data->mip_widths);
+ free(tex_data->mip_heights);
+ free(tex_data);
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (num_mip_levels > 1 && mip_widths != nullptr && mip_heights != nullptr) { | |
| // Mipmapped path: compute total data size across all levels | |
| size_t total_size = 0; | |
| for (int i = 0; i < num_mip_levels; ++i) { | |
| total_size += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | |
| } | |
| void* all_data = malloc(total_size); | |
| if (all_data == nullptr) { | |
| free(tex_data); | |
| return false; | |
| } | |
| memcpy(all_data, data, total_size); | |
| // Allocate mipmap arrays | |
| tex_data->mip_data = (void**)malloc(sizeof(void*) * num_mip_levels); | |
| tex_data->mip_widths = (int*)malloc(sizeof(int) * num_mip_levels); | |
| tex_data->mip_heights = (int*)malloc(sizeof(int) * num_mip_levels); | |
| // Set up per-level pointers into the contiguous buffer | |
| char* ptr = (char*)all_data; | |
| for (int i = 0; i < num_mip_levels; ++i) { | |
| tex_data->mip_data[i] = ptr; | |
| tex_data->mip_widths[i] = mip_widths[i]; | |
| tex_data->mip_heights[i] = mip_heights[i]; | |
| ptr += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | |
| } | |
| if (num_mip_levels > 1 && mip_widths != nullptr && mip_heights != nullptr) { | |
| // Mipmapped path: compute total data size across all levels | |
| size_t total_size = 0; | |
| for (int i = 0; i < num_mip_levels; ++i) { | |
| total_size += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | |
| } | |
| void* all_data = malloc(total_size); | |
| if (all_data == nullptr) { | |
| free(tex_data); | |
| return false; | |
| } | |
| memcpy(all_data, data, total_size); | |
| // Allocate mipmap arrays | |
| tex_data->mip_data = (void**)malloc(sizeof(void*) * num_mip_levels); | |
| tex_data->mip_widths = (int*)malloc(sizeof(int) * num_mip_levels); | |
| tex_data->mip_heights = (int*)malloc(sizeof(int) * num_mip_levels); | |
| if (tex_data->mip_data == nullptr || tex_data->mip_widths == nullptr || tex_data->mip_heights == nullptr) { | |
| free(all_data); | |
| free(tex_data->mip_data); | |
| free(tex_data->mip_widths); | |
| free(tex_data->mip_heights); | |
| free(tex_data); | |
| return false; | |
| } | |
| // Set up per-level pointers into the contiguous buffer | |
| char* ptr = (char*)all_data; | |
| for (int i = 0; i < num_mip_levels; ++i) { | |
| tex_data->mip_data[i] = ptr; | |
| tex_data->mip_widths[i] = mip_widths[i]; | |
| tex_data->mip_heights[i] = mip_heights[i]; | |
| ptr += (size_t)mip_widths[i] * mip_heights[i] * num_channels * bytes_per_channel; | |
| } |
🤖 Prompt for AI Agents
In `@warp/native/texture.cpp` around lines 144 - 170, The three mallocs that
create tex_data->mip_data, tex_data->mip_widths and tex_data->mip_heights in the
mipmapped 2D path (and the analogous three mallocs in the 3D mipmapped path)
must be null-checked: after each malloc, verify the returned pointer is
non-null; on any failure free any prior successful allocations (free
tex_data->mip_data, tex_data->mip_widths, tex_data->mip_heights if set), free
all_data, and return false (also ensure you do not dereference null pointers
afterward); apply the same pattern for the 3D mipmap allocation block so both
mipmapped paths safely clean up and return false on allocation failure.
| // mip_filter_mode: 0=nearest, 1=linear | ||
| // address_mode_u, address_mode_v: 0=wrap, 1=clamp, 2=mirror, 3=border (per-axis) | ||
| // use_normalized_coords: if true, texture coordinates are in [0,1]; if false, in texel space [0,width/height] | ||
| // num_mip_levels: number of mip levels to create | ||
| // mip_widths, mip_heights: array of mip widths and heights (must be power of two) | ||
| WP_API bool wp_texture2d_create_device( | ||
| void* context, | ||
| int width, | ||
| int height, | ||
| int num_channels, | ||
| int dtype, | ||
| int filter_mode, | ||
| int mip_filter_mode, | ||
| int address_mode_u, | ||
| int address_mode_v, | ||
| bool use_normalized_coords, | ||
| int num_mip_levels, | ||
| const void* data, | ||
| const int* mip_widths, | ||
| const int* mip_heights, | ||
| uint64_t* tex_handle_out, | ||
| uint64_t* array_handle_out | ||
| uint64_t* array_handle_out, | ||
| uint64_t* mipmap_handle_out | ||
| ); |
There was a problem hiding this comment.
Comment claims "must be power of two" but this isn't enforced.
Lines 198 and 262 state mip widths/heights/depths "must be power of two", but neither the C++ implementation nor the Python side validates this constraint. If this is a CUDA hardware requirement, consider adding validation; otherwise, update the comment to reflect the actual contract (e.g., "typically halved at each level").
| def test_mipmap2d_max_load(test, device): | ||
| """Test that sampling at the highest LOAD returns approximately the average of all texels.""" | ||
| width, height = 4, 4 | ||
| # Create a gradient pattern | ||
| data = np.arange(16, dtype=np.float32).reshape((4, 4)) | ||
|
|
There was a problem hiding this comment.
Remove unused width and height variables.
width and height are assigned but never referenced — the data shape is hardcoded as (4, 4). Flagged by Ruff (F841).
Suggested fix
- width, height = 4, 4
- # Create a gradient pattern
- data = np.arange(16, dtype=np.float32).reshape((4, 4))
+ # Create a gradient pattern (4x4)
+ data = np.arange(16, dtype=np.float32).reshape((4, 4))Alternatively, use the variables in the reshape:
width, height = 4, 4
# Create a gradient pattern
- data = np.arange(16, dtype=np.float32).reshape((4, 4))
+ data = np.arange(width * height, dtype=np.float32).reshape((height, width))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_mipmap2d_max_load(test, device): | |
| """Test that sampling at the highest LOAD returns approximately the average of all texels.""" | |
| width, height = 4, 4 | |
| # Create a gradient pattern | |
| data = np.arange(16, dtype=np.float32).reshape((4, 4)) | |
| def test_mipmap2d_max_load(test, device): | |
| """Test that sampling at the highest LOAD returns approximately the average of all texels.""" | |
| # Create a gradient pattern (4x4) | |
| data = np.arange(16, dtype=np.float32).reshape((4, 4)) |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 2206-2206: Local variable width is assigned to but never used
Remove assignment to unused variable width
(F841)
[error] 2206-2206: Local variable height is assigned to but never used
Remove assignment to unused variable height
(F841)
🤖 Prompt for AI Agents
In `@warp/tests/cuda/test_texture.py` around lines 2204 - 2209, The
test_mipmap2d_max_load function currently defines unused variables width and
height while hardcoding the reshape to (4, 4); remove the unused assignments or
use them in the data.reshape call to fix the Ruff F841 warning. Locate the
function test_mipmap2d_max_load and either delete the lines "width, height = 4,
4" or change "data = np.arange(16, dtype=np.float32).reshape((4, 4))" to use the
width and height variables (e.g., reshape((height, width))) so the variables are
referenced.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
warp/_src/builtins.py (2)
7619-7641:⚠️ Potential issue | 🟡 MinorRemove default values from
loddocstrings.The
loddescriptions repeat the default0.0already shown by Sphinx; please drop the explicit default text.🧹 Suggested docstring tweak
- lod: Level of detail for mipmapped textures. Default is 0.0 (no mipmaps). + lod: Level of detail for mipmapped textures.- lod: Level of detail for mipmapped textures. Default is 0.0 (no mipmaps). + lod: Level of detail for mipmapped textures.- lod: Level of detail for mipmapped textures. Default is 0.0 (no mipmaps). + lod: Level of detail for mipmapped textures.- lod: Level of detail for mipmapped textures. Default is 0.0 (no mipmaps). + lod: Level of detail for mipmapped textures.As per coding guidelines, "Don't repeat default values from function signatures in docstrings—Sphinx autodoc shows them automatically".
Also applies to: 7645-7669, 7692-7714, 7718-7744
7619-7626:⚠️ Potential issue | 🟡 MinorRegenerate
warp/__init__.pyiafter modifying builtins.Please run
build_docs.pyso the public stubs reflect the newlodparameter.As per coding guidelines, "After modifying
warp/_src/builtins.py, runbuild_docs.pyto regeneratewarp/__init__.pyi".
🤖 Fix all issues with AI agents
In `@warp/native/texture.h`:
- Around line 542-546: The CPU sampling paths for vector types pass a float lod
into cpu_sample_2d_channel (which takes an int) causing truncation and skipping
inter-level interpolation; update the vec2f::sample_2d, vec2f::sample_3d,
vec4f::sample_2d, and vec4f::sample_3d branches so that when lod > 0.0f they
call the mip-aware helpers (cpu_sample_2d_channel_mip /
cpu_sample_3d_channel_mip) with the float LOD to perform proper fractional mip
interpolation instead of calling cpu_sample_2d_channel / cpu_sample_3d_channel
directly. Ensure you replace the non-mip calls in those four methods with their
corresponding _mip variants.
- Around line 496-500: The cpu sampling branch currently calls
cpu_sample_2d_channel(cpu_tex, u, v, 0, 0) when lod <= 0.0 which can dereference
tex->mip_widths/mip_heights for single-level textures; change the call to pass
level = -1 so cpu_sample_2d_channel uses the base texture dimensions instead of
accessing mip arrays, and make the same change for the other CPU sampling call
sites mentioned (the spots that call cpu_sample_2d_channel with level 0 in the
non-mip branch); keep the mip path using cpu_sample_2d_channel_mip(cpu_tex, u,
v, 0, lod) unchanged.
🧹 Nitpick comments (1)
warp/tests/cuda/test_texture.py (1)
2084-2098: Consistent "LOAD" → "LOD" typo in comments and docstrings throughout mipmap tests.The PR commit message explicitly renames "load" to "lod", but many comments and docstrings still say "LOAD" (e.g., "Sample at LOAD 0", "LOAD 1", "max LOAD", "test_mipmap2d_integer_load", "test_mipmap2d_max_load"). While the actual parameter is correctly named
lod, the documentation is inconsistent.Examples: Lines 2152, 2167–2168, 2170, 2185, 2204–2205, 2218, 2221, 2255, 2372, 2404.
| if (lod <= 0.0) { | ||
| return cpu_sample_2d_channel(cpu_tex, u, v, 0, 0); | ||
| } else { | ||
| return cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "mip_data|mip_widths|mip_heights|num_mip_levels" warp/native/texture.cpp | head -80Repository: NVIDIA/warp
Length of output: 4306
🏁 Script executed:
rg -A 20 "cpu_sample_2d_channel\(" warp/native/texture.cpp | head -60Repository: NVIDIA/warp
Length of output: 37
🏁 Script executed:
rg -n -A 15 "cpu_sample_2d_channel\(" warp/native/texture.hRepository: NVIDIA/warp
Length of output: 4137
Fix null pointer dereference for non-mipmapped textures in CPU sampling code.
The code at line 497 calls cpu_sample_2d_channel(cpu_tex, u, v, 0, 0) with level=0 when lod <= 0.0. However, the cpu_sample_2d_channel function (line 284) accesses tex->mip_widths[level] and tex->mip_heights[level] when level >= 0. For single-level textures (num_mip_levels=1), these mip arrays are set to nullptr in texture.cpp (lines 186-190), causing a null pointer dereference.
Pass level=-1 instead of level=0 in the lod <= 0.0 branch so it uses the base texture dimensions rather than attempting to access the unallocated mip arrays. Apply the same fix at lines 543, 545, 594-600.
🤖 Prompt for AI Agents
In `@warp/native/texture.h` around lines 496 - 500, The cpu sampling branch
currently calls cpu_sample_2d_channel(cpu_tex, u, v, 0, 0) when lod <= 0.0 which
can dereference tex->mip_widths/mip_heights for single-level textures; change
the call to pass level = -1 so cpu_sample_2d_channel uses the base texture
dimensions instead of accessing mip arrays, and make the same change for the
other CPU sampling call sites mentioned (the spots that call
cpu_sample_2d_channel with level 0 in the non-mip branch); keep the mip path
using cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod) unchanged.
| if (lod <= 0.0f) { | ||
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, 0), cpu_sample_2d_channel(cpu_tex, u, v, 1, 0)); | ||
| } else { | ||
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod)); | ||
| } |
There was a problem hiding this comment.
Bug: vec2f CPU mip path passes float lod to cpu_sample_2d_channel (expects int level), skipping inter-level interpolation.
The float specialization correctly calls cpu_sample_2d_channel_mip for lod > 0, but the vec2f specialization calls cpu_sample_2d_channel directly, passing the float lod which silently truncates to int. This means fractional LODs won't interpolate between mip levels for 2-channel textures on CPU.
The same bug exists in:
vec2f::sample_3d(Lines 567–569)vec4f::sample_2d(Lines 598–601)vec4f::sample_3d(Lines 627–629)
🐛 Proposed fix for vec2f::sample_2d (apply same pattern to all four affected paths)
} else {
- return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod));
+ return vec2f(cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 1, lod));
}For vec4f::sample_2d:
} else {
return vec4f(
- cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod),
- cpu_sample_2d_channel(cpu_tex, u, v, 2, lod), cpu_sample_2d_channel(cpu_tex, u, v, 3, lod)
+ cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 1, lod),
+ cpu_sample_2d_channel_mip(cpu_tex, u, v, 2, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 3, lod)
);
}Apply the analogous _mip change for the 3D variants (sample_3d) in both vec2f and vec4f.
🤖 Prompt for AI Agents
In `@warp/native/texture.h` around lines 542 - 546, The CPU sampling paths for
vector types pass a float lod into cpu_sample_2d_channel (which takes an int)
causing truncation and skipping inter-level interpolation; update the
vec2f::sample_2d, vec2f::sample_3d, vec4f::sample_2d, and vec4f::sample_3d
branches so that when lod > 0.0f they call the mip-aware helpers
(cpu_sample_2d_channel_mip / cpu_sample_3d_channel_mip) with the float LOD to
perform proper fractional mip interpolation instead of calling
cpu_sample_2d_channel / cpu_sample_3d_channel directly. Ensure you replace the
non-mip calls in those four methods with their corresponding _mip variants.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
warp/__init__.pyi (1)
4674-4690:⚠️ Potential issue | 🟠 Major
lodparameter should have a default value in stub signatures to match implementation.The actual implementation (warp/_src/builtins.py) defines all four
texture_sampleoverloads withdefaults={"lod": 0.0}, but the stub signatures declarelod: float32as required. This causes type checkers to incorrectly flag existing call sites that omitlod, breaking backward compatibility. The docstring correctly states "Default is 0.0 (no mipmaps)" but the signature doesn't reflect this.Proposed fix (apply to all four overloads)
-def texture_sample(tex: Texture2D, uv: vec2f, dtype: Any, lod: float32) -> Any: +def texture_sample(tex: Texture2D, uv: vec2f, dtype: Any, lod: float32 = ...) -> Any:-def texture_sample(tex: Texture2D, u: float32, v: float32, dtype: Any, lod: float32) -> Any: +def texture_sample(tex: Texture2D, u: float32, v: float32, dtype: Any, lod: float32 = ...) -> Any:-def texture_sample(tex: Texture3D, uvw: vec3f, dtype: Any, lod: float32) -> Any: +def texture_sample(tex: Texture3D, uvw: vec3f, dtype: Any, lod: float32 = ...) -> Any:-def texture_sample(tex: Texture3D, u: float32, v: float32, w: float32, dtype: Any, lod: float32) -> Any: +def texture_sample(tex: Texture3D, u: float32, v: float32, w: float32, dtype: Any, lod: float32 = ...) -> Any:
| float coord_u = tex->use_normalized_coords ? u : (u / (float)tex->width); | ||
| float coord_v = tex->use_normalized_coords ? v : (v / (float)tex->height); |
There was a problem hiding this comment.
unnormalized coords use base texture dims instead of mip dims
| float coord_u = tex->use_normalized_coords ? u : (u / (float)tex->width); | |
| float coord_v = tex->use_normalized_coords ? v : (v / (float)tex->height); | |
| float coord_u = tex->use_normalized_coords ? u : (u / (float)width); | |
| float coord_v = tex->use_normalized_coords ? v : (v / (float)height); |
| float coord_u = tex->use_normalized_coords ? u : (u / (float)tex->width); | ||
| float coord_v = tex->use_normalized_coords ? v : (v / (float)tex->height); | ||
| float coord_w = tex->use_normalized_coords ? w : (w / (float)tex->depth); |
There was a problem hiding this comment.
unnormalized coords use base texture dims instead of mip dims
| float coord_u = tex->use_normalized_coords ? u : (u / (float)tex->width); | |
| float coord_v = tex->use_normalized_coords ? v : (v / (float)tex->height); | |
| float coord_w = tex->use_normalized_coords ? w : (w / (float)tex->depth); | |
| float coord_u = tex->use_normalized_coords ? u : (u / (float)width); | |
| float coord_v = tex->use_normalized_coords ? v : (v / (float)height); | |
| float coord_w = tex->use_normalized_coords ? w : (w / (float)depth); |
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod)); | ||
| } |
There was a problem hiding this comment.
float lod truncated to int, bypasses mip filter interpolation. CPU disagrees with CUDA
| return vec2f(cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod)); | |
| } | |
| return vec2f(cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 1, lod)); | |
| } |
| return vec2f( | ||
| cpu_sample_3d_channel(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 1, lod) | ||
| ); |
There was a problem hiding this comment.
float lod truncated to int, bypasses mip filter interpolation. CPU disagrees with CUDA
| return vec2f( | |
| cpu_sample_3d_channel(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 1, lod) | |
| ); | |
| return vec2f( | |
| cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 1, lod) | |
| ); |
| return vec4f( | ||
| cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod), | ||
| cpu_sample_2d_channel(cpu_tex, u, v, 2, lod), cpu_sample_2d_channel(cpu_tex, u, v, 3, lod) | ||
| ); |
There was a problem hiding this comment.
float lod truncated to int, bypasses mip filter interpolation. CPU disagrees with CUDA
| return vec4f( | |
| cpu_sample_2d_channel(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel(cpu_tex, u, v, 1, lod), | |
| cpu_sample_2d_channel(cpu_tex, u, v, 2, lod), cpu_sample_2d_channel(cpu_tex, u, v, 3, lod) | |
| ); | |
| return vec4f( | |
| cpu_sample_2d_channel_mip(cpu_tex, u, v, 0, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 1, lod), | |
| cpu_sample_2d_channel_mip(cpu_tex, u, v, 2, lod), cpu_sample_2d_channel_mip(cpu_tex, u, v, 3, lod) | |
| ); |
| return vec4f( | ||
| cpu_sample_3d_channel(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 1, lod), | ||
| cpu_sample_3d_channel(cpu_tex, u, v, w, 2, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 3, lod) | ||
| ); |
There was a problem hiding this comment.
float lod truncated to int, bypasses mip filter interpolation. CPU disagrees with CUDA
| return vec4f( | |
| cpu_sample_3d_channel(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 1, lod), | |
| cpu_sample_3d_channel(cpu_tex, u, v, w, 2, lod), cpu_sample_3d_channel(cpu_tex, u, v, w, 3, lod) | |
| ); | |
| return vec4f( | |
| cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 0, lod), cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 1, lod), | |
| cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 2, lod), cpu_sample_3d_channel_mip(cpu_tex, u, v, w, 3, lod) | |
| ); |
Additional Comments (1)
|
|
The feature sounds useful and the proposed API looks reasonable. Is the MR already ready for review? I still see a couple of TODO. |
|
Great! I can finalize the MR then if it looks reasonable to implement it this way. Give me a couple days and I can get it ready for review |
Thanks! FYI, the cutoff for changes going into v1.12.0 is February 23, so the changes should be merged into |
3 participants
Description
For downstream applications, adding mipmap support to the current texture API allows for better texture sampling for detailed textures at various distances. CUDA textures already support this but some modifications need to be done to correctly create and pass in the texture data.
This PR adds:
Example without mip support for a plane with a checkerboard texture with border between squares:
After mip support passing lod (level of detail) into texture sample function:
Downstream the API usage changes from this:
to this:
TODO:
Before your PR is "Ready for review"
__init__.pyi,docs/api_reference/,docs/language_reference/)pre-commit run -aSummary by CodeRabbit
New Features
Tests