Realtime Compositor: add regression tests #109878
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#109878
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "zazizizou/blender:com-realtime-regression-test"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Tests must be enabled manually using the CMake flag
WITH_COMPOSITOR_REALTIME_TEST
. Reasons are F12 for realtime compositor is experimental and buildbots have no GPU.Notes about the test cases:
BLACKLIST
Todo after this patch lands:
Are we duplicating the CPU compositor tests? I though we were going to reuse them and just change the compositor evaluator to the realtime one.
@OmarEmaraDev my reasoning for duplicating blend files was 1) duplicate blend files is better than duplicating code and 2) I wanted to remove test cases for unsupported nodes.
Looking deeper into the
Report
class I found out I was wrong about that, so this patch now uses the same blend files, but different reference images for each compositor.@ -699,2 +699,4 @@
)
endforeach()
if(WITH_COMPOSITOR_REALTIME_TEST)
This seems duplicated below.
@ -0,0 +35,4 @@
"-F", "PNG",
"-f", "1"]
if compositor_dirname == 'compositor_realtime':
Those changes are no longer needed?
I didn't solve conflicts properly when merging from main, sorry. Cleanup imports should also be a separate PR. Should be fixed now
@ -0,0 +15,4 @@
except ImportError:
inside_blender = False
NOT_IMPLEMENTED = [
I shall submit a patch for this soon, so I would just leave this out.
In the future, we should also share the reference images between the CPU and GPU. But it is reasonable to separate them for now until we unify the compositors.
Can you also check with Sergey to review the patch? As I can't solely approve it myself.
@OmarEmaraDev I thought I would add Sergey as soon as you approve
if(WITH_COMPOSITOR_REALTIME_TEST)
is now insideif(WITH_COMPOSITOR_CPU)
, which is unnecessary as far as I can see.You can't add compositor nodes if
WITH_COMPOSITOR_CPU
isOFF
, so I think viewport compositor does depend on cpu compositor.Are you sure? I can't seem to find a dependency between
WITH_COMPOSITOR_CPU
and compositor nodes.@OmarEmaraDev I added
matte
anddistortion
tests and identified a few more crashing and failing tests. For now they are disabled but I want to investigate further.I am still not able to run the tests successfully when disabling
WITH_COMPOSITOR_CPU
, so for now it must be set toON
.@ -24,6 +24,7 @@ set(WITH_PYTHON_SAFETY ON CACHE BOOL "" FORCE)
if(WIN32)
set(WITH_WINDOWS_BUNDLE_CRT OFF CACHE BOOL "" FORCE)
endif()
set(WITH_COMPOSITOR_REALTIME_TEST ON CACHE BOOL "" FORCE)
I am not sure about this. The other GPU related tests (
WITH_OPENGL_RENDER_TESTS
andWITH_OPENGL_DRAW_TESTS
) are not enabled for the developer configuration.Makes me wonder why is it so. I remember there were some issues caused by different samplers on different GPUs, which used to cause "false-positive" failures. Not sure if that was resolved.
@fclem can you share your insights here?
Yep, that isn't resolved. Maybe you will have a better time with the compositor tests. IIRC the color space ones were the one that were failing.
Thanks Clément!
@zazizizou How do you feel about not enabling
WITH_COMPOSITOR_REALTIME_TEST
for the developer configuration just yet, get the rest the patch landed. Then we can run the tests on various hardware and see how it behaves, and once we are comfortable we enable it for the developer config?Thanks Clément!
@zazizizou How do you feel about not enabling
WITH_COMPOSITOR_REALTIME_TEST
for the developer configuration just yet, get the rest the patch landed. Then we can run the tests on various hardware and see how it behaves, and once we are comfortable we enable it for the developer config?@Sergey @fclem Thanks for looking into this !
For me it's important to run these tests for each patch with the compositor. If they are not active in buildbot by default then it doesn't make much of a difference for me whether it's active for developer config or not, we need to rely on developer discipline anyway to run the tests before committing.
I will update the patch accordingly :)
Currently we can only run those tests on macOS, as other platforms do not have GPUs. This is how we do it for Cycles, i.e., and it is handled outside of the developer configuration. Once we know the tests do not cause false-positive failures we will enable them on the buildbot.
Sounds good to me, thanks!
Sounds good to me, thanks!
having single set of .blend files and different reference images is the easiest way to go forward. Hopefully in the future we will be able to use a single set of reference images, but there are some things to be taken care of before this can happen. Until then I think this is the way to go.
As for the
WITH_COMPOSITOR_CPU
the "issue" is that it is theCOM_execute
which invokesRE_compositor_execute
, and the former one is within#ifdef WITH_COMPOSITOR_CPU
check. While it is unideal and could be confusing, I would suggest against unentangling it as part of this PR.The main uncertainty for me is whether we (well, me) feel comfortable enabling
WITH_COMPOSITOR_REALTIME_TEST
for the developer configuration. Is something I don't have enough experience w.r.t how reliable different GPUs are, and I don't want to run into situation when people just "learn" to ignore GPU compositor failures.Other than that, as long as we agree on a single set of .blend files and a separate set of reference images I think this PR is pretty much good to go.
Simple one-liner I've noticed which needs to be addressed.
Was testing the patch locally, and that is the biggest stopper so far.
Form the remaining things:
WITH_COMPOSITOR_REALTIME_TEST
for the developer configuration just yet.And think we'll be ready to land.
P.S. It is also interesting to compare render results of the realtime and COU compositors. Maybe some things are not implemented yet, but others are definitely interesting to understand why they are different. But this we should do after the ground work for tests is landed.
@ -695,2 +694,3 @@
${CMAKE_CURRENT_LIST_DIR}/compositor_cpu_render_tests.py
-blender "${TEST_BLENDER_EXE}"
-testdir "${TEST_SRC_DIR}/compositor/${comp_test}"
-testdir "${TEST_SRC_DIR}/compositor_cpu/${comp_test}"
This needs to remain to be
-testdir "${TEST_SRC_DIR}/compositor/${comp_test}"
, and only the reference images are to be stored in a different folder. Same as what is done for render tests (cycles_renders
vs.eevee_renders
).I think doing this simple tweak and running tests with
BLENDER_TEST_UPDATE=1
will bring us to the the state believe is what we want in the main branch.Thanks for the updates.
Some minor comment, otherwise seems fine. If you're fine with the suggestion just update the patch, but there is no need to wait the next round off review.
@ -728,0 +729,4 @@
endif()
endif()
# XXX WITH_COMPOSITOR_CPU is needed for rendering.
I would replace XXX with NOTE. XXX is something bad in the code right below, but here it is just a "quirk" of how defines work in Blender.
@Sergey, I'm really liking the idea of these tests, but I'm finding on my environment, I'm not getting the tests to pull the test data from the lib directory, (or pull the updated data down from the cloud) which is making all the tests break for me. Looking at the report HTML, I'm seeing all the "new" images, but the ref and diff images fail to populate. I'd prefer to see this handled in CMake, but I'm pretty green with CMake still. All suggestions welcome.
@linux_dr We had some issues with svn checkout on buildbot machines: somehow simple
svn up
left the checkout in a dirty state, leading to the similar sounding behavior. Doingsvn revert -R .
resolved the problem. Maybe you're running into the same quirk.If that is not it, it might be better to talk in the blender chat instead, where more people who are familiar with cmake and svn can chime in and help.
@Sergey, Where is the blender developer chat now? I lost track of it after the IRC network ownership drama last year.
@linux_dr https://blender.chat/channel/blender-coders/
Issue #111455 closed!