Skip to content

Volume selection and edge detection possible improvement #201

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.

Sign up for GitHub

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

Closed
favreau wants to merge 1 commit into from
+108 −21
Closed

Volume selection and edge detection possible improvement #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 106 additions & 18 deletions devices/rtx/device/gpu/volumeIntegration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,20 @@ VISRTX_DEVICE const SpatialFieldGPUData &getSpatialFieldData(
}

VISRTX_DEVICE void volumeSamplerInit(
VolumeSamplingState *samplerState,
const SpatialFieldGPUData &field)
VolumeSamplingState *samplerState, const SpatialFieldGPUData &field)
{
optixDirectCall<void>(
uint32_t(field.samplerCallableIndex) + static_cast<uint32_t>(SpatialFieldSamplerEntryPoints::Init),
optixDirectCall<void>(uint32_t(field.samplerCallableIndex)
+ static_cast<uint32_t>(SpatialFieldSamplerEntryPoints::Init),
samplerState,
&field);
}

VISRTX_DEVICE float volumeSamplerSample(
const VolumeSamplingState *samplerState,
VISRTX_DEVICE float volumeSamplerSample(const VolumeSamplingState *samplerState,
const SpatialFieldGPUData &field,
const vec3 &position)
{
return optixDirectCall<float>(
uint32_t(field.samplerCallableIndex) + static_cast<uint32_t>(SpatialFieldSamplerEntryPoints::Sample),
return optixDirectCall<float>(uint32_t(field.samplerCallableIndex)
+ static_cast<uint32_t>(SpatialFieldSamplerEntryPoints::Sample),
samplerState,
&position);
}
Expand All @@ -95,12 +93,30 @@ VISRTX_DEVICE vec4 classifySample(const VolumeGPUData &v, float s)
return retval;
}

Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The colorToMaterialID function lacks documentation explaining its purpose, parameters, and return value. Given its role in material identification for edge detection, a docstring would clarify why RGB quantization is used and how the numBins parameter affects the result.

Suggested change
// Map an RGB color to a discrete material identifier.
// The input color is assumed to be in [0, 1] per channel and is first
// clamped to that range. Each channel is then quantized into @p numBins
// uniformly spaced bins, and the three quantized components are packed
// into a single 32-bit material ID as:
// ID = r * numBins^2 + g * numBins + b
//
// This provides up to numBins^3 distinct material IDs and is typically
// used to assign stable, discrete IDs for operations such as edge or
// material detection based on color.
//
// @param color RGB color in linear space, each component typically in [0, 1].
// @param numBins Number of quantization bins per channel; higher values
// increase color resolution and the number of possible IDs.
// @return A packed material ID derived from the quantized RGB color.

Copilot uses AI. Check for mistakes.
VISRTX_DEVICE uint32_t colorToMaterialID(
const vec3 &color, uint32_t numBins = 16)
{
// Quantize RGB color to create a material ID
// Each channel is divided into numBins levels
const uint32_t r =
static_cast<uint32_t>(glm::clamp(color.x, 0.f, 1.f) * (numBins - 1));
const uint32_t g =
static_cast<uint32_t>(glm::clamp(color.y, 0.f, 1.f) * (numBins - 1));
const uint32_t b =
static_cast<uint32_t>(glm::clamp(color.z, 0.f, 1.f) * (numBins - 1));

// Pack into a single ID: each channel gets log2(numBins) bits
return (r * numBins * numBins) + (g * numBins) + b;
}

VISRTX_DEVICE void _rayMarchVolume(ScreenSample &ss,
const VolumeHit &hit,
box1 interval,
vec3 *color,
float &opacity,
float invSamplingRate)
float invSamplingRate,
float *outDepth = nullptr,
uint32_t *outMaterialID = nullptr)
{
const auto &volume = *hit.volume;
/////////////////////////////////////////////////////////////////////////////
Expand All @@ -125,6 +141,9 @@ VISRTX_DEVICE void _rayMarchVolume(ScreenSample &ss,
curand_uniform(&ss.rs) * (interval.upper - interval.lower);

constexpr float OPACITY_THRESHOLD = 0.99f;
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The DEPTH_OPACITY_THRESHOLD (0.01f = 1%) differs from MIN_SEGMENT_OPACITY (0.001f = 0.1%) used for objID assignment on line 392. These serve similar purposes (determining visibility) but use different thresholds. Consider consolidating to a single named constant or documenting why different thresholds are needed.

Suggested change
constexpr float OPACITY_THRESHOLD = 0.99f;
constexpr float OPACITY_THRESHOLD = 0.99f;
// Note: This depth threshold (1% opacity) is intentionally higher than the
// minimum segment opacity used elsewhere for objID assignment (0.1%). For
// depth/material reporting we only consider the volume "hit" once it has a
// clearly visible contribution along the ray, to avoid noisy, near-transparent
// samples influencing the first-hit depth and material ID.

Copilot uses AI. Check for mistakes.
constexpr float DEPTH_OPACITY_THRESHOLD = 0.01f;
bool depthSet = false;
bool materialIDSet = false;

while (opacity < OPACITY_THRESHOLD && size(interval) >= 0.f) {
const vec3 p = hit.localRay.org + hit.localRay.dir * interval.lower;
Expand All @@ -138,6 +157,19 @@ VISRTX_DEVICE void _rayMarchVolume(ScreenSample &ss,
if (color)
*color += transmittance * stepOpacity * vec3(co);
opacity += transmittance * stepOpacity;

// Track depth and material ID of first opaque sample
if (!depthSet && opacity >= DEPTH_OPACITY_THRESHOLD) {
if (outDepth) {
*outDepth = interval.lower;
depthSet = true;
}
if (outMaterialID && !materialIDSet) {
// Use transfer function color to determine material ID
*outMaterialID = colorToMaterialID(vec3(co));
materialIDSet = true;
}
}
}

interval.lower += stepSize;
Expand All @@ -148,7 +180,8 @@ VISRTX_DEVICE float rayMarchVolume(ScreenSample &ss,
const VolumeHit &hit,
vec3 *color,
float &opacity,
float invSamplingRate)
float invSamplingRate,
uint32_t *outMaterialID = nullptr)
{
const auto &volume = *hit.volume;
/////////////////////////////////////////////////////////////////////////////
Expand All @@ -158,10 +191,18 @@ VISRTX_DEVICE float rayMarchVolume(ScreenSample &ss,
/////////////////////////////////////////////////////////////////////////////
const float stepSize = volume.stepSize;
box1 interval = hit.localRay.t;
const float depth = interval.lower;
float depth =
hit.localRay.t.upper; // Default to far plane (no opaque content)
interval.lower += stepSize * curand_uniform(&ss.rs); // jitter

_rayMarchVolume(ss, hit, interval, color, opacity, invSamplingRate);
_rayMarchVolume(ss,
hit,
interval,
color,
opacity,
invSamplingRate,
&depth,
outMaterialID);

return depth;
}
Expand Down Expand Up @@ -290,32 +331,78 @@ VISRTX_DEVICE float rayMarchAllVolumes(ScreenSample &ss,
VolumeHit hit;
ray.t.upper = tfar;
float depth = tfar;
float contentDepth = tfar; // Track content-based depth separately

constexpr float OPACITY_THRESHOLD = 0.99f;

objID = ~0;
instID = ~0;
opacity = 0.0f;
uint32_t materialID = ~0; // Track material ID from first opaque sample
uint32_t volumeID = ~0; // Track the volume ID for objID assignment
bool objIDSet =
false; // Track if we've already set objID based on first visible sample

do {
hit.foundHit = false;
intersectVolume(ss, ray, type, &hit);
if (!hit.foundHit)
break;
if (objID == ~0) {
objID = hit.volume->id;
if (instID == ~0) {
instID = hit.instance->id;
}
if (volumeID == ~0) {
volumeID = hit.volume->id;
}

// Track closest intersection depth
// Track closest bounding box intersection depth
depth = min(depth, hit.localRay.t.lower);

// Save where this volume ends, so
// float tmax = hit.localRay.t.upper;
hit.localRay.t.upper = glm::min(tfar, hit.localRay.t.upper);

// Ray march through this volume segment
detail::rayMarchVolume(ss, hit, &color, opacity, invSamplingRate);
// Ray march through this volume segment and get content depth
float segmentContentDepth = hit.localRay.t.upper; // Default to far plane
vec3 segmentColor(0.f);
float segmentOpacity = 0.f;
uint32_t segmentMaterialID = ~0;

const auto &volume = *hit.volume;
const float stepSize = volume.stepSize;
box1 interval = hit.localRay.t;
interval.lower += stepSize * curand_uniform(&ss.rs); // jitter

detail::_rayMarchVolume(ss,
hit,
interval,
&segmentColor,
segmentOpacity,
invSamplingRate,
&segmentContentDepth,
&segmentMaterialID);

// Composite the segment color
const float transmittance = (1.0f - opacity);
Copy link
Collaborator

@tarcila tarcila Jan 19, 2026

Choose a reason for hiding this comment

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

Feels like we are trying to reimplement part of the existing logic of detail::rayMarchVolume. I feel most of the changes could be done there instead.

color += transmittance * segmentColor;
opacity += transmittance * segmentOpacity;

// Set objID based on first visible sample (before full accumulation)
// Use a very low threshold to ensure consistent objID
constexpr float MIN_SEGMENT_OPACITY = 0.001f;
if (!objIDSet && segmentOpacity > MIN_SEGMENT_OPACITY && volumeID != ~0u) {
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The condition checks volumeID != ~0u but volumeID is initialized to ~0 (which defaults to int type) on line 342. This comparison should use ~0 instead of ~0u for type consistency, or volumeID should be explicitly initialized as ~0u.

Suggested change
if (!objIDSet && segmentOpacity > MIN_SEGMENT_OPACITY && volumeID != ~0u) {
if (!objIDSet && segmentOpacity > MIN_SEGMENT_OPACITY && volumeID != ~0) {

Copilot uses AI. Check for mistakes.
objID = volumeID;
objIDSet = true;
}

// Update content depth and material ID if this is the first significant
// opacity contribution
if (contentDepth >= tfar && segmentContentDepth < hit.localRay.t.upper) {
contentDepth = segmentContentDepth;
if (segmentMaterialID != ~0u) {
materialID = segmentMaterialID;
Copy link
Collaborator

@tarcila tarcila Jan 19, 2026

Choose a reason for hiding this comment

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

That one does not seem to be unused. It would need to be dropped and the related code.

}
}

if (ray.t.lower < hit.localRay.t.upper)
ray.t.lower = hit.localRay.t.upper;
Expand All @@ -324,7 +411,8 @@ VISRTX_DEVICE float rayMarchAllVolumes(ScreenSample &ss,

} while (opacity < OPACITY_THRESHOLD);

return depth;
// Return content-based depth if opaque content was found, otherwise far plane
return contentDepth;
}

template <typename RAY_TYPE>
Expand Down
3 changes: 1 addition & 2 deletions tsd/src/tsd/rendering/pipeline/passes/VisualizeAOVPass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ void computeEdgesImage(
return;
}

// Check if any neighbor has a different object ID (including
// background)
// Check if any neighbor has a different object ID
Copy link
Collaborator

@tarcila tarcila Jan 19, 2026

Choose a reason for hiding this comment

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

Was the previous comment wrong? The related code doesn't look it changed.

bool isEdge = false;

for (int dy = -1; dy <= 1 && !isEdge; ++dy) {
Expand Down
2 changes: 1 addition & 1 deletion tsd/src/tsd/ui/imgui/windows/Viewport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ struct Viewport : public Window
bool m_showAxes{true};
float m_depthVisualMinimum{0.f};
float m_depthVisualMaximum{1.f};
float m_edgeThreshold{0.5f};
float m_edgeThreshold{0.5f}; // 0.0 = 1px radius, 1.0 = 5px radius
Copy link
Collaborator

@tarcila tarcila Jan 19, 2026

Choose a reason for hiding this comment

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

Does not seem to be used elsewhere in this PR. Experiments left-over?

bool m_edgeInvert{false};

float m_fov{40.f};
Expand Down