studio/flamenco
studio
/
flamenco
12
27
Fork 32
Code Issues 60 Pull Requests 7 Releases Wiki Activity

WIP: Single-frame job compiler #104194

Draft
k8ie wants to merge 30 commits from k8ie/flamenco:single-frame into main
pull from: k8ie/flamenco:single-frame
merge into: studio:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 18 Commits 30 Files Changed 1 +350
k8ie commented 2023-03-06 14:50:03 +01:00
First-time contributor
Copy Link

Same as #104189, aims to implement #104188. I accidentally closed the original PR by typing into the wrong window. I am very sorry about that.

Current task list:

  • Split the logic for the tiling into a function so that I can use it again for denoising

  • Denoising - I'll reuse the above mentioned tiling logic

  • The tiling logic math could use splitting into human-understandable variable names

  • Replace os with pathlib

  • Create helper variables for bpy.context.scene, etc.

  • Reject when not using Cycles

  • Fix merging being totally broken when using denoising (don't know the cause)

  • Don't hard-code EXR layer names, they might change between Blender versions

  • Don't hard-code the scene name, the scene can be renamed by the user

Same as #104189, aims to implement #104188. I accidentally closed the original PR by typing into the wrong window. I am very sorry about that. Current task list: - [x] Split the logic for the tiling into a function so that I can use it again for denoising - [x] Denoising - I'll reuse the above mentioned tiling logic - [x] The tiling logic math could use splitting into human-understandable variable names - [x] Replace `os` with `pathlib` - [x] Create helper variables for `bpy.context.scene`, etc. - [ ] Reject when not using Cycles - [ ] Fix merging being totally broken when using denoising (don't know the cause) - [ ] Don't hard-code EXR layer names, they might change between Blender versions - [ ] Don't hard-code the scene name, the scene can be renamed by the user
k8ie added 7 commits 2023-03-06 14:50:05 +01:00
k8ie force-pushed single-frame from 46a83336f5 to b9aca93b54 2023-03-06 14:50:34 +01:00 Compare
k8ie added 1 commit 2023-03-06 14:56:18 +01:00
k8ie closed this pull request 2023-03-06 15:07:34 +01:00
k8ie reopened this pull request 2023-03-06 15:07:57 +01:00
Sybren A. Stüvel commented 2023-03-07 17:35:12 +01:00
Owner
Copy Link

No worries, things like that happen.

When you want me to review, be sure to remove the WIP: prefix from the title.

No worries, things like that happen. When you want me to review, be sure to remove the `WIP:` prefix from the title.
👍 1
k8ie added 1 commit 2023-03-13 19:59:19 +01:00
k8ie added 1 commit 2023-03-13 21:34:38 +01:00
k8ie added 1 commit 2023-03-14 00:03:24 +01:00
k8ie added 1 commit 2023-03-15 17:45:52 +01:00
k8ie added 1 commit 2023-03-16 13:36:12 +01:00
k8ie added 1 commit 2023-03-16 16:15:17 +01:00
k8ie commented 2023-03-16 16:45:33 +01:00
Author
First-time contributor
Copy Link

Hi, I rewrote the chunking to be tile-based.

Overall this seems to work very well, path guiding works as expected. I still don't have overlap between tiles for adaptive sampling to work because I don't know how exactly I would go about cropping them before the merge.

There is one regression that bothers me quite a bit. I switched to using the compositor to merge the image. The node network I use looks like this: image

The problem with this approach is that in order for the merge to happen, Blender has to keep all the files in memory. In a test with a 1080p image split into 100 tiles, the memory usage for a merge was ~4GB. 12GB for the same image but with denoising.

I thought about splitting the merging into separate tasks, but I don't know if that would be the ideal approach especially when you compare Blender's startup time with the amount of time it takes to merge two tiles.

There is another issue, and that is keeping the denoising data from the individual tiles. I'm not aware of any way to get the denoising data back into a multi-layer OpenEXR outside of the "File Output" node. This node allows adding an arbitrary amount of input sockets which will be stored in layers in the EXR file. I think I've read that the File Output node is a pretty hacky thing to use, so I would like to avoid using it if possible.

Hi, I rewrote the chunking to be tile-based. Overall this seems to work very well, path guiding works as expected. I still don't have overlap between tiles for adaptive sampling to work because I don't know how exactly I would go about cropping them before the merge. There is one regression that bothers me quite a bit. I switched to using the compositor to merge the image. The node network I use looks like this: ![image](/attachments/d8eb3a5f-446c-499c-8d31-0b832d0182cf) The problem with this approach is that in order for the merge to happen, Blender has to keep all the files in memory. In a test with a 1080p image split into 100 tiles, the memory usage for a merge was ~4GB. 12GB for the same image but with denoising. I thought about splitting the merging into separate tasks, but I don't know if that would be the ideal approach especially when you compare Blender's startup time with the amount of time it takes to merge two tiles. There is another issue, and that is keeping the denoising data from the individual tiles. I'm not aware of any way to get the denoising data back into a multi-layer OpenEXR outside of the "File Output" node. This node allows adding an arbitrary amount of input sockets which will be stored in layers in the EXR file. I think I've read that the File Output node is a pretty hacky thing to use, so I would like to avoid using it if possible.
image.png
890 KiB
k8ie added 1 commit 2023-03-16 16:55:05 +01:00
Sybren A. Stüvel commented 2023-03-20 12:12:53 +01:00
Owner
Copy Link

You made some nice progress!

About the memory usage, I think bpy.context.scene.render.use_crop_to_border = False is the culprit. If you set that to True, the image files will be much smaller, and only contain the rendered pixels. Of course you'll get a bit more complex flow with the merging, as you have to position each image correctly.

As for the Python code, there are some things to adjust.

  • Avoid copy-pasting bpy.context.... everywhere. It's harder to read, and it's also slower for Python to execute (but that doesn't matter too much in the grander scheme of things). If you need repeated access to the scene, just use scene = bpy.context.scene and then use scene. Same for accessing the node tree, etc.
  • Something similar for repeated calculations. There are a lot of repetitions of ${settings.tile_size} / 100, so just calculate that once, put the result in a variable, and use that instead. Having said that, I think it might be better to just input the desired number of tiles? The JS code can then determine how to split up the frame into that many tiles by itself. This avoids having to deal with remainders, like passing 33% as the tile size, but that leading to four tiles (3x 33% + 1x 1%).
  • Don't overwrite the renderer. If this approach can only work with Cycles, then just make the JavaScript reject the job when it uses any other renderer. How to do that gracefully is another thing, but silently switching render engines is not really nice to the artist.
  • Avoid string manipulation and os.path for handling paths. Restricting yourself to pathlib.Path will reduce the number of APIs you use, and make things simpler to read and understand. And it'll be less fragile. To make things a little shorter, you can use from pathlib import Path.
You made some nice progress! About the memory usage, I think `bpy.context.scene.render.use_crop_to_border = False` is the culprit. If you set that to `True`, the image files will be much smaller, and only contain the rendered pixels. Of course you'll get a bit more complex flow with the merging, as you have to position each image correctly. As for the Python code, there are some things to adjust. - Avoid copy-pasting `bpy.context....` everywhere. It's harder to read, and it's also slower for Python to execute (but that doesn't matter too much in the grander scheme of things). If you need repeated access to the scene, just use `scene = bpy.context.scene` and then use `scene`. Same for accessing the node tree, etc. - Something similar for repeated calculations. There are a lot of repetitions of `${settings.tile_size} / 100`, so just calculate that once, put the result in a variable, and use that instead. Having said that, I think it might be better to just input the desired number of tiles? The JS code can then determine how to split up the frame into that many tiles by itself. This avoids having to deal with remainders, like passing 33% as the tile size, but that leading to four tiles (3x 33% + 1x 1%). - Don't overwrite the renderer. If this approach can only work with Cycles, then just make the JavaScript reject the job when it uses any other renderer. How to do that gracefully is another thing, but silently switching render engines is not really nice to the artist. - Avoid string manipulation and `os.path` for handling paths. Restricting yourself to `pathlib.Path` will reduce the number of APIs you use, and make things simpler to read and understand. And it'll be less fragile. To make things a little shorter, you can use `from pathlib import Path`.
Sybren A. Stüvel requested changes 2023-03-20 12:16:37 +01:00
Sybren A. Stüvel left a comment
Owner
Copy Link

.

.
k8ie commented 2023-03-27 14:22:17 +02:00
Author
First-time contributor
Copy Link

Hi, thanks for the feedback. You're probably right about the tile cropping and memory usage, fixing that is priority number one for me at the moment.

Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.

Hi, thanks for the feedback. You're probably right about the tile cropping and memory usage, fixing that is priority number one for me at the moment. Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.
Sybren A. Stüvel commented 2023-03-27 15:48:29 +02:00
Owner
Copy Link

Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged.

That's fine, you working at a slow pace on this is still infinitely faster than me not doing anything at all here ;-)

> Sorry about the development being slower and slower, I have a bunch of IRL stuff that keeps me pretty busy, but rest assured, I'd still like to finish implementing this and get this merged. That's fine, you working at a slow pace on this is still infinitely faster than me not doing anything at all here ;-)
❤️ 1
k8ie added 1 commit 2023-03-29 18:35:32 +02:00
k8ie commented 2023-03-29 18:43:46 +02:00
Author
First-time contributor
Copy Link

Alright, I finally got the memory usage under control. I still have a few things to do tho.

  • Split the logic for the tiling into a function so that I can use it again for denoising
  • Denoising - I'll reuse the above mentioned tiling logic
  • The tiling logic math could use splitting into human-understandable variable names
  • Replace os with pathlib
  • Create helper variables for bpy.context.scene, etc.
  • Reject when not using Cycles
Alright, I finally got the memory usage under control. I still have a few things to do tho. - Split the logic for the tiling into a function so that I can use it again for denoising - Denoising - I'll reuse the above mentioned tiling logic - The tiling logic math could use splitting into human-understandable variable names - Replace `os` with `pathlib` - Create helper variables for `bpy.context.scene`, etc. - Reject when not using Cycles
k8ie commented 2023-03-29 18:49:52 +02:00
Author
First-time contributor
Copy Link

Oh, and looks like the merging doesn't work as expected for some tile sizes (interestingly enough, only the nice ones that fit the image exactly like 50%, 10%, etc.).

  • I guess I'll have to fix that first 😅
Oh, and looks like the merging doesn't work as expected for some tile sizes (interestingly enough, only the nice ones that fit the image exactly like 50%, 10%, etc.). - [x] I guess I'll have to fix that first 😅
k8ie added 1 commit 2023-03-29 21:02:22 +02:00
k8ie added 1 commit 2023-03-29 21:10:22 +02:00
Sybren A. Stüvel commented 2023-03-30 16:04:05 +02:00
Owner
Copy Link

Please don't track sub-tasks in a comment, just edit the PR description and put them there. That way Gitea recognises them (it shows like 3/5 in the PR list) and your local reviewer doesn't have to scroll up & down to find them ;-)

Please don't track sub-tasks in a comment, just edit the PR description and put them there. That way Gitea recognises them (it shows like `3/5` in the PR list) and your local reviewer doesn't have to scroll up & down to find them ;-)
k8ie commented 2023-03-30 17:02:32 +02:00
Author
First-time contributor
Copy Link

Oh, ok, that makes sense, sorry 😅

Oh, ok, that makes sense, sorry 😅
Sybren A. Stüvel commented 2023-03-30 17:03:10 +02:00
Owner
Copy Link

Is faaaaaaaaaaaaaaain, we all learn all the time ;-)

Is faaaaaaaaaaaaaaain, we all learn all the time ;-)
👍 1
k8ie added 2 commits 2023-04-01 15:42:40 +02:00
k8ie added 1 commit 2023-04-01 16:39:15 +02:00
k8ie added 1 commit 2023-04-01 17:22:16 +02:00
k8ie commented 2023-04-01 17:41:04 +02:00
Author
First-time contributor
Copy Link

Alright, I think I'm pretty much done with the implementation of denoising.

There is an issue I ran into though. When combining the Denoising Albedos and Normals, the results are glitchy.
What's even weirder is that every time I run the composite, the result is glitched out in a different way.
I'll attach the two tiles I'm testing with.

This node network:
image

Results in this image:
image

For the complete context, this is the full node network I use:
image

Do you have any idea why this could be happening?

Alright, I think I'm pretty much done with the implementation of denoising. There is an issue I ran into though. When combining the Denoising Albedos and Normals, the results are glitchy. What's even weirder is that every time I run the composite, the result is glitched out in a different way. I'll attach the two tiles I'm testing with. This node network: ![image](/attachments/5bf06cab-6854-4663-be27-8b1365341d35) Results in this image: ![image](/attachments/afb4118b-9df1-48c9-9d7a-bdc679a1b358) For the complete context, this is the full node network I use: ![image](/attachments/c4723c40-eacd-4f0a-95cb-4de00bc6e18b) Do you have any idea why this could be happening?
image.png
1.0 MiB
image.png
447 KiB
image.png
632 KiB
r0c0_0670.exr
9.8 MiB
r0c1_0670.exr
12 MiB
r0c0_0670.exr r0c1_0670.exr
k8ie added 1 commit 2023-04-04 15:55:22 +02:00
k8ie added 2 commits 2023-04-04 16:25:56 +02:00
k8ie added 1 commit 2023-04-04 16:30:45 +02:00
Sybren A. Stüvel commented 2023-07-06 12:53:03 +02:00
Owner
Copy Link

Do you have any idea why this could be happening?

Sorry, I don't have the time to dive into this myself.

> Do you have any idea why this could be happening? Sorry, I don't have the time to dive into this myself.
k8ie commented 2023-07-07 19:05:17 +02:00
Author
First-time contributor
Copy Link

@dr.sybren No worries ;)

@dr.sybren No worries ;)
k8ie added 1 commit 2023-07-07 19:05:51 +02:00
k8ie added 1 commit 2023-07-08 19:43:05 +02:00
k8ie added 1 commit 2023-07-08 19:47:18 +02:00
k8ie added 1 commit 2023-07-09 13:24:55 +02:00
Sybren A. Stüvel commented 2023-11-06 11:25:55 +01:00
Owner
Copy Link

Hey! How's the progress? The PR is still marked as 'work in progress', so I haven't looked at it in depth. Is there anything that's blocking you from removing the 'WIP' prefix?

Hey! How's the progress? The PR is still marked as 'work in progress', so I haven't looked at it in depth. Is there anything that's blocking you from removing the 'WIP' prefix?
k8ie commented 2023-11-06 13:32:57 +01:00
Author
First-time contributor
Copy Link

Hey, I haven't worked on this for a while.

Last time I checked, I still had this happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included.

Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do)

Otherwise I believe it should be pretty much ready. Just some cleanup here and there. Nothing huge other than that pesky tiling issue.

Hey, I haven't worked on this for a while. Last time I checked, I still had [this](https://projects.blender.org/studio/flamenco/pulls/104194#issuecomment-912820) happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included. Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do) Otherwise I believe it should be pretty much ready. Just some cleanup here and there. Nothing huge other than that pesky tiling issue.
Sybren A. Stüvel commented 2023-11-06 14:39:27 +01:00
Owner
Copy Link

Last time I checked, I still had this happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included.

Did you file a bug report for Blender itself? If not, then this isn't going to get fixed. Blender developers (well, except me) aren't going to check this repository to find bugs. So if you haven't, please report it.

Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do)

It would help to keep track of what still needs doing in the PR description. That way its current status is clear for anyone looking at it (without having to read through each comment in detail).

> Last time I checked, I still had [this](https://projects.blender.org/studio/flamenco/pulls/104194#issuecomment-912820) happening when tiling denoising data together. I (hopefully) left enough info in that comment for anyone to be able to try it manually in Blender and reproduce the issue. The two EXR files used are included. Did you file a bug report for Blender itself? If not, then this isn't going to get fixed. Blender developers (well, except me) aren't going to check this repository to find bugs. So if you haven't, please report it. > Unrelated - I think the layer names may have changed in EXRs somewhere between Blender versions, I'll need to account for that. (I'll add that to the original comment, should be pretty easy to do) It would help to keep track of what still needs doing in the PR description. That way its current status is clear for anyone looking at it (without having to read through each comment in detail).
k8ie commented 2023-11-06 15:40:05 +01:00
Author
First-time contributor
Copy Link

I didn't file a bug report, I'll try doing that hopefully this week. Thanks for the pointer.

It would help to keep track of what still needs doing in the PR description.

Yup, that's what I meant 😉
It's added in there as a checkbox.

I didn't file a bug report, I'll try doing that hopefully this week. Thanks for the pointer. > It would help to keep track of what still needs doing in the PR description. Yup, that's what I meant 😉 It's added in there as a checkbox.
k8ie commented 2023-12-07 08:40:28 +01:00
Author
First-time contributor
Copy Link

@dr.sybren I have reported the issue upstream, we'll see what we learn 😉

@dr.sybren I have reported the issue upstream, we'll see what we learn 😉
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u single-frame:k8ie-single-frame
git checkout k8ie-single-frame

Merge

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff k8ie-single-frame
git checkout main
git merge --ff-only k8ie-single-frame
git checkout k8ie-single-frame
git rebase main
git checkout main
git merge --no-ff k8ie-single-frame
git checkout main
git merge --squash k8ie-single-frame
git checkout main
git merge k8ie-single-frame
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
Good First Issue
  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Confirmed

  
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 Label
Good First Issue
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Confirmed
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
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 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: studio/flamenco#104194
No description provided.
Delete Branch "k8ie/flamenco:single-frame"

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?