Fix #111277: NaN in Vector Displacement leading to render errors #111294

Merged
Sergey Sharybin merged 2 commits from Alaska/blender:fix-111277 into main 2023-08-21 15:22:12 +02:00
Member

Fixes NaN in Vector Displacement node caused by the normalization of
0, 0, 0 vectors.

This fixes both visual rendering issues and an "illegal address" error
on the GPU. The "illegal address" error came from the Light Tree
Sampling code not handling the NaN normals well, leading to weird code
paths being taken, eventually leading to a kernel_assert and a
user facing illegal address error.

Fixes NaN in Vector Displacement node caused by the normalization of 0, 0, 0 vectors. This fixes both visual rendering issues and an "illegal address" error on the GPU. The "illegal address" error came from the Light Tree Sampling code not handling the NaN normals well, leading to weird code paths being taken, eventually leading to a kernel_assert and a user facing illegal address error.
Alaska added the
Module
Render & Cycles
label 2023-08-19 12:20:55 +02:00
Alaska added 1 commit 2023-08-19 12:21:06 +02:00
9b4e01bef1 Fix 111277: NaN in Vector Displacement leading to render errors
Fixes NaN in Vector Displacement node caused by the normalization of
0, 0, 0 vectors.
Author
Member

If this commit gets accepted, it needs to be backported to Blender 3.3 and 3.6.

Note 1: This fixes both the issues reported in 111277. It fixes the black faces issues (NaN normals), and the illegal address issue on CUDA.

Note 2: OSL does not need updating to use safe_normalize(). OSL always uses safe_normalize() when normalize() is called. EEVEE on the other hand already seemed to handle this case fine but I updated it to also use safe_normalize() just in case.

If this commit gets accepted, it needs to be backported to Blender 3.3 and 3.6. Note 1: This fixes both the issues reported in 111277. It fixes the black faces issues (NaN normals), and the illegal address issue on CUDA. Note 2: OSL does not need updating to use `safe_normalize()`. OSL always uses `safe_normalize()` when `normalize()` is called. EEVEE on the other hand already seemed to handle this case fine but I updated it to also use `safe_normalize()` just in case.
Alaska requested review from Brecht Van Lommel 2023-08-19 12:34:09 +02:00
Alaska requested review from Weizhen Huang 2023-08-19 12:34:16 +02:00
Alaska requested review from Sergey Sharybin 2023-08-19 12:34:24 +02:00
Alaska requested review from Lukas Stockner 2023-08-19 12:34:32 +02:00
Alaska changed title from Fix 111277: NaN in Vector Displacement leading to render errors to Fix #111277: NaN in Vector Displacement leading to render errors 2023-08-19 12:35:18 +02:00
Alaska added 1 commit 2023-08-19 13:08:28 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
31f72a9b1c
Update EEVEE

@Alaska it should not be a problem to back-port to LTS releases, assuming code applies cleanly :)

Before landing to the main branch I am very curious about that Illegal CUDA Address you've mentioned. How is it NaN causing an invalid address issue?
It might be very good idea to mention this aspect of a fix, and perhaps elaborate on it in the PR description (which will become a commit message).

@Alaska it should not be a problem to back-port to LTS releases, assuming code applies cleanly :) Before landing to the main branch I am very curious about that Illegal CUDA Address you've mentioned. How is it NaN causing an invalid address issue? It might be very good idea to mention this aspect of a fix, and perhaps elaborate on it in the PR description (which will become a commit message).

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR111294) when ready.
Author
Member

Before landing to the main branch I am very curious about that Illegal CUDA Address you've mentioned. How is it NaN causing an invalid address issue?
It might be very good idea to mention this aspect of a fix, and perhaps elaborate on it in the PR description (which will become a commit message).

@Sergey, the NaN normals was leading to an issue where a light wasn't being selected in the light tree sampling code, leading to the default -1 index light being selected, which results in a kernel_assert because -1 was outside the range of the light storage being accessed, and this appears as an "illegal address" error to the user.

I added a catch case for when a light isn't selected (Which only appears to happen when a NaN ends up in the light sampling code) in this pull request: #111292

Basically, the NaN normals isn't causing the illegal address, the NaN normals was causing weird code paths to be taken in the light tree sampling code resulting in an illegal address error.


I'm not sure if I should include this in the commit message or not, but I've added an explanation anyway.

> Before landing to the main branch I am very curious about that Illegal CUDA Address you've mentioned. How is it NaN causing an invalid address issue? > It might be very good idea to mention this aspect of a fix, and perhaps elaborate on it in the PR description (which will become a commit message). @Sergey, the NaN normals was leading to an issue where a light wasn't being selected in the light tree sampling code, leading to the default -1 index light being selected, which results in a kernel_assert because -1 was outside the range of the light storage being accessed, and this appears as an "illegal address" error to the user. I added a catch case for when a light isn't selected (Which only appears to happen when a NaN ends up in the light sampling code) in this pull request: #111292 Basically, the NaN normals isn't causing the illegal address, the NaN normals was causing weird code paths to be taken in the light tree sampling code resulting in an illegal address error. --- I'm not sure if I should include this in the commit message or not, but I've added an explanation anyway.
Sergey Sharybin approved these changes 2023-08-21 15:20:36 +02:00
Sergey Sharybin left a comment
Owner

Thanks for the explanation!

And I do think it makes sense in the commit message.

Thanks for the explanation! And I do think it makes sense in the commit message.
Sergey Sharybin merged commit 7f65080ab4 into main 2023-08-21 15:22:12 +02:00
Sergey Sharybin deleted branch fix-111277 2023-08-21 15:22:14 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111294
No description provided.