WIP: Single-frame job compiler #104194

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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
First-time contributor

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

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.
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
Author
First-time contributor

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.
890 KiB
k8ie added 1 commit 2023-03-16 16:55:05 +01:00

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

.

.
Author
First-time contributor

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.

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 ;-)
k8ie added 1 commit 2023-03-29 18:35:32 +02:00
Author
First-time contributor

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
Author
First-time contributor

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

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 ;-)
Author
First-time contributor

Oh, ok, that makes sense, sorry 😅

Oh, ok, that makes sense, sorry 😅

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

Is faaaaaaaaaaaaaaain, we all learn all the time ;-)
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
Author
First-time contributor

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

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.
Author
First-time contributor

@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

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?
Author
First-time contributor

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.

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).
Author
First-time contributor

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.
Author
First-time contributor

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

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.
No description provided.