Realtime Compositor: add regression tests #109878

Merged
Habib Gahbiche merged 12 commits from zazizizou/blender:com-realtime-regression-test into main 2023-08-19 15:01:01 +02:00
Member

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:

  • Crashing nodes are not included, see BLACKLIST
  • Some nodes didn't work for me. I didn't include those and reported the problems as bugs, see #109869 and #109868

Todo after this patch lands:

  • Update tests in svn
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: - Crashing nodes are not included, see `BLACKLIST` - Some nodes didn't work for me. I didn't include those and reported the problems as bugs, see #109869 and #109868 Todo after this patch lands: - [x] Update tests in svn
Habib Gahbiche added 1 commit 2023-07-09 19:14:24 +02:00
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.
Habib Gahbiche requested review from Omar Emara 2023-07-09 19:14:41 +02:00
Habib Gahbiche added 1 commit 2023-07-10 00:50:23 +02:00
Member

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.

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.
Sergey Sharybin added this to the Compositing project 2023-07-14 16:20:01 +02:00
Habib Gahbiche added 2 commits 2023-07-23 22:03:52 +02:00
Author
Member

@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.

@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.
Omar Emara requested changes 2023-07-24 09:35:16 +02:00
@ -699,2 +699,4 @@
)
endforeach()
if(WITH_COMPOSITOR_REALTIME_TEST)
Member

This seems duplicated below.

This seems duplicated below.
zazizizou marked this conversation as resolved
@ -0,0 +35,4 @@
"-F", "PNG",
"-f", "1"]
if compositor_dirname == 'compositor_realtime':
Member

Those changes are no longer needed?

Those changes are no longer needed?
Author
Member

I didn't solve conflicts properly when merging from main, sorry. Cleanup imports should also be a separate PR. Should be fixed now

I didn't solve conflicts properly when merging from main, sorry. Cleanup imports should also be a separate PR. Should be fixed now
zazizizou marked this conversation as resolved
@ -0,0 +15,4 @@
except ImportError:
inside_blender = False
NOT_IMPLEMENTED = [
Member

I shall submit a patch for this soon, so I would just leave this out.

I shall submit a patch for this soon, so I would just leave this out.
zazizizou marked this conversation as resolved
Member

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.

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.
Habib Gahbiche added 1 commit 2023-07-24 20:46:08 +02:00
Author
Member

@OmarEmaraDev I thought I would add Sergey as soon as you approve

@OmarEmaraDev I thought I would add Sergey as soon as you approve
Member

if(WITH_COMPOSITOR_REALTIME_TEST) is now inside if(WITH_COMPOSITOR_CPU), which is unnecessary as far as I can see.

`if(WITH_COMPOSITOR_REALTIME_TEST)` is now inside `if(WITH_COMPOSITOR_CPU)`, which is unnecessary as far as I can see.
Author
Member

You can't add compositor nodes if WITH_COMPOSITOR_CPU is OFF, so I think viewport compositor does depend on cpu compositor.

You can't add compositor nodes if `WITH_COMPOSITOR_CPU` is `OFF`, so I think viewport compositor does depend on cpu compositor.
Member

Are you sure? I can't seem to find a dependency between WITH_COMPOSITOR_CPU and compositor nodes.

Are you sure? I can't seem to find a dependency between `WITH_COMPOSITOR_CPU` and compositor nodes.
Habib Gahbiche added 2 commits 2023-08-06 15:35:14 +02:00
Author
Member

@OmarEmaraDev I added matte and distortion 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 to ON.

@OmarEmaraDev I added `matte` and `distortion` 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 to `ON`.
Habib Gahbiche requested review from Omar Emara 2023-08-06 15:42:17 +02:00
Sergey Sharybin reviewed 2023-08-09 16:18:13 +02:00
@ -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 and WITH_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?

I am not sure about this. The other GPU related tests (`WITH_OPENGL_RENDER_TESTS` and `WITH_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.

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?

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?
Author
Member

@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 :)

@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.

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.
Author
Member

Sounds good to me, thanks!

Sounds good to me, thanks!
Author
Member

Sounds good to me, thanks!

Sounds good to me, thanks!
zazizizou marked this conversation as resolved

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 the COM_execute which invokes RE_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.

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 the `COM_execute` which invokes `RE_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.
Sergey Sharybin requested changes 2023-08-11 15:41:18 +02:00
Sergey Sharybin left a comment
Owner

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:

  • Don't enable WITH_COMPOSITOR_REALTIME_TEST for the developer configuration just yet.
  • Address the one-liner I've just found

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.

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: - Don't enable `WITH_COMPOSITOR_REALTIME_TEST ` for the developer configuration just yet. - Address the one-liner I've just found 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.

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.
zazizizou marked this conversation as resolved
Habib Gahbiche added 2 commits 2023-08-12 12:36:51 +02:00
Habib Gahbiche added 1 commit 2023-08-12 12:54:24 +02:00
Sergey Sharybin approved these changes 2023-08-14 11:30:48 +02:00
Sergey Sharybin left a comment
Owner

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.

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.

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.
zazizizou marked this conversation as resolved
Habib Gahbiche added 2 commits 2023-08-19 14:58:19 +02:00
Habib Gahbiche merged commit 9d08be1eec into main 2023-08-19 15:01:01 +02:00
Habib Gahbiche deleted branch com-realtime-regression-test 2023-08-19 15:01:03 +02:00
First-time contributor

@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.

@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. Doing svn 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.

@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. Doing `svn 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.
First-time contributor

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.

> _…_ 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/
First-time contributor

Issue #111455 closed!

Issue #111455 closed!
Sergey Sharybin removed this from the Compositing project 2023-09-06 10:38:01 +02:00
Sign in to join this conversation.
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
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#109878
No description provided.