DRW: remove edges hidden by Optimal Display in IBO creation #106402

Merged
Germano Cavalcante merged 3 commits from mano-wii/blender:drw_ibo_lines_opnimal_display_removal into main 2023-04-03 15:59:53 +02:00

No significant functional changes.

Edges hidden by Optimal Display are hidden by edge factor Shader.
But there is not much advantage in doing this, as the number of edges
hidden by the Optimal Display is usually much higher than the number of
visible edges.

And the lines extractor does not include invisible edges due to other
factors.

This change also makes the code better match the lines extraction code
for GPU Subdivision.

No significant functional changes. Edges hidden by Optimal Display are hidden by edge factor Shader. But there is not much advantage in doing this, as the number of edges hidden by the Optimal Display is usually much higher than the number of visible edges. And the lines extractor does not include invisible edges due to other factors. This change also makes the code better match the lines extraction code for GPU Subdivision.
Germano Cavalcante added the
Module
EEVEE & Viewport
label 2023-03-31 19:43:03 +02:00
Germano Cavalcante added 1 commit 2023-03-31 19:43:08 +02:00
cd53c9c557 DRW: remove edges hidden by Optimal Display in IBO creation
No significant functional changes.

Edges hidden by Optimal Display are hidden by edge factor Shader.
But there is not much advantage in doing this, as the number of edges
hidden by the Optimal Display is usually much higher than the number of
visible edges.

And the lines extractor does not include invisible edges due to other
factors.

This change also makes the code better match the lines extracting code
for GPU Subdivision.
Hans Goudey requested review from Hans Goudey 2023-03-31 19:48:06 +02:00
Germano Cavalcante requested review from Jeroen Bakker 2023-03-31 19:52:17 +02:00
Hans Goudey reviewed 2023-03-31 22:24:52 +02:00
Hans Goudey left a comment
Member

No significant functional changes.

Are there insignificant functional changes? :) Might this be an optimization since later parts of the process that might be skipped when they're filtered out here?

The code looks good, but I think it would be nice to have a clearer description of the benefits of the change.

>No significant functional changes. Are there **in**significant functional changes? :) Might this be an optimization since later parts of the process that might be skipped when they're filtered out here? The code looks good, but I think it would be nice to have a clearer description of the benefits of the change.
@ -20,0 +33,4 @@
if (data->e_origindex && data->e_origindex[edge] == ORIGINDEX_NONE) {
return false;
}
if (!data->optimal_display_edges.is_empty() && data->optimal_display_edges[edge] == 0) {
Member

data->optimal_display_edges[edge] == 0 -> !data->optimal_display_edges[edge]

Since operator[] gives a BitRef

`data->optimal_display_edges[edge] == 0` -> `!data->optimal_display_edges[edge]` Since `operator[]` gives a `BitRef`
mano-wii marked this conversation as resolved
Author
Member

Better to just say "No functional changes". In fact only a micro-optimization can be felt.

About optimization, IBO extraction should not change at all. Perhaps drawing can be a few tenths of FPS faster.

I tweaked it because I noticed differences in GPU and CPU Subdivision codes.
It will be good to match the codes to fix some bugs that were caught with #90641

Better to just say "No functional changes". In fact only a micro-optimization can be felt. About optimization, IBO extraction should not change at all. Perhaps drawing can be a few tenths of FPS faster. I tweaked it because I noticed differences in GPU and CPU Subdivision codes. It will be good to match the codes to fix some bugs that were caught with https://projects.blender.org/blender/blender/issues/90641
Germano Cavalcante added 2 commits 2023-03-31 22:44:05 +02:00
Member

Okay, thanks for clarifying a bit. Can checking optimal_display_edges in extract_edge_fac be skipped now though? It seems like you're pointing out some redundancy in the patch description when you say "But there is not much advantage in doing this"

Perhaps drawing can be a few tenths of FPS faster

I realize this PR probably won't make a difference. But in general I'd think it would be best to measure the cost of the extraction directly in seconds, with a timer in extract_task_range_run_iter for example.

Okay, thanks for clarifying a bit. Can checking `optimal_display_edges` in `extract_edge_fac` be skipped now though? It seems like you're pointing out some redundancy in the patch description when you say "But there is not much advantage in doing this" >Perhaps drawing can be a few tenths of FPS faster I realize this PR probably won't make a difference. But in general I'd think it would be best to measure the cost of the extraction directly in seconds, with a timer in `extract_task_range_run_iter` for example.
Jeroen Bakker approved these changes 2023-04-03 08:44:35 +02:00
Author
Member

I tried to get the results of profiling CPU extraction, GPU render. But the results were so close that I couldn't really spot the difference.

I took the average of extract_task_range_run_iter and the results:

patch: 6.41565 ms
main:  6.41954 ms

So the advantage of this change is more for code quality.

  • Visibility test is consistent with what is actually seen.
  • Smaller buffer for IBO sent to GPU
  • consistency with GPU Subdivision that already considers Optimal Display
  • Allows possible improvement in the "Edge Factor" extraction by making it unnecessary to check the Optimal Display (except for optimization cases).
I tried to get the results of profiling CPU extraction, GPU render. But the results were so close that I couldn't really spot the difference. I took the average of `extract_task_range_run_iter` and the results: ``` patch: 6.41565 ms main: 6.41954 ms ``` So the advantage of this change is more for code quality. - Visibility test is consistent with what is actually seen. - Smaller buffer for IBO sent to GPU - consistency with GPU Subdivision that already considers `Optimal Display` - Allows possible improvement in the "Edge Factor" extraction by making it unnecessary to check the `Optimal Display` (except for optimization cases).
Hans Goudey approved these changes 2023-04-03 15:24:29 +02:00
Germano Cavalcante merged commit d1fe11c79f into main 2023-04-03 15:59:53 +02:00
Germano Cavalcante deleted branch drw_ibo_lines_opnimal_display_removal 2023-04-03 15:59:54 +02:00
Sign in to join this conversation.
No reviewers
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
Interest: X11
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#106402
No description provided.