blender/blender-asset-tracer
2
5
Fork 5
Code Issues 4 Pull Requests 1 Projects Releases Wiki Activity

Add Support for Geometry Node Cache #92890

Open
Jonas Dichelle wants to merge 14 commits from JonasDichelle/blender-asset-tracer:geonodes_support into main
pull from: JonasDichelle/blender-asset-tracer:geonodes_support
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 1 Commits 14 Files Changed 6 +95 -2
Jonas Dichelle commented 2024-09-03 00:52:08 +02:00
Contributor
Copy Link

This PR adds support for tracking the geometry node cache through a custom iterator because modifier.bakes is a pointer to a dynamic array.
Currently, BAT interprets the target of a pointer as a single block. Because dynamic arrays are defined only by a single pointer, they are decoded like this:

<BlendFileBlock.NodesModifierBake (DATA), size=144 at 0x7f4ec1510c28>
This is an array of three bakes, each of size 48, decoded as a single block of size 144 (48 * 3).

So, without changing the struct decoding behavior of BAT, this seems to be the best way to parse the array:

  • Copy the large NodesModifierBake block for each element of the array.
  • Adjust the size and file offset of each new block.

To do this, I added a new iterator called dynamic_array that yields blocks representing each element within the dynamic array.

Unless I'm mistaken, it looks like dynamic arrays aren't typically used for structs like this in Blender, which is likely why BAT hasn't had to handle them before.

I added a zip to test this PR:

This PR adds support for tracking the geometry node cache through a custom iterator because `modifier.bakes` is a pointer to a dynamic array.\ Currently, BAT interprets the target of a pointer as a single block. Because dynamic arrays are defined only by a single pointer, they are decoded like this: > `<BlendFileBlock.NodesModifierBake (DATA), size=144 at 0x7f4ec1510c28>`\ > This is an array of three bakes, each of size 48, decoded as a single block of size 144 (48 * 3). So, without changing the struct decoding behavior of BAT, this seems to be the best way to parse the array: - Copy the large `NodesModifierBake` block for each element of the array. - Adjust the size and file offset of each new block. To do this, I added a new iterator called `dynamic_array` that yields blocks representing each element within the dynamic array. Unless I'm mistaken, it looks like dynamic arrays aren't typically used for structs like this in Blender, which is likely why BAT hasn't had to handle them before. <sub>I added a zip to test this PR:</sub>
geonodetest.zip
41 MiB
👍 1
Jonas Dichelle added 12 commits 2024-09-03 00:52:09 +02:00
Jonas Dichelle requested review from Sybren A. Stüvel 2024-09-03 00:53:22 +02:00
Sybren A. Stüvel requested changes 2024-09-05 11:39:12 +02:00
Sybren A. Stüvel left a comment
Member
Copy Link

Thanks for the PR! I've left some inline notes.

Could you also add some unit tests that cover these changes? Especially since you introduce the support for dynamic arrays (which is very nice to have in there), I feel that this should be tested properly (also to ensure it doesn't break in future updates).

Thanks for the PR! I've left some inline notes. Could you also add some unit tests that cover these changes? Especially since you introduce the support for dynamic arrays (which is very nice to have in there), I feel that this should be tested properly (also to ensure it doesn't break in future updates).
.gitignore Outdated
@ -12,2 +12,4 @@
/docs/_build
/.env
Sybren A. Stüvel commented 2024-09-05 11:21:50 +02:00
Member
Copy Link

The use of these directories is quite personal, and differs from developer to developer. Please put those into your .git/info/exclude file. That behaves like an extra .gitignore but isn't tracked by Git.

The use of these directories is quite personal, and differs from developer to developer. Please put those into your `.git/info/exclude` file. That behaves like an extra `.gitignore` but isn't tracked by Git.
JonasDichelle marked this conversation as resolved
blender_asset_tracer/blendfile/__init__.py Outdated
@ -130,3 +130,1 @@
self.code_index = collections.defaultdict(
list
) # type: typing.Dict[bytes, BFBList]
self.code_index = collections.defaultdict(list) # type: typing.Dict[bytes, BFBList]
Sybren A. Stüvel commented 2024-09-05 11:23:01 +02:00
Member
Copy Link

Please keep formatting changes out of this PR. There is no direct need for this PR to modify this file.

Of course non-functional improvements like this are always welcome, but shouldn't be part of the same PR as functional changes.

Please keep formatting changes out of this PR. There is no direct need for this PR to modify this file. Of course non-functional improvements like this are always welcome, but shouldn't be part of the same PR as functional changes.
JonasDichelle marked this conversation as resolved
blender_asset_tracer/blendfile/iterators.py
@ -72,1 +72,4 @@
yield from listbase(mods, next_path=(b"modifier", b"next"))
def copy_block(block: BlendFileBlock) -> BlendFileBlock:
Sybren A. Stüvel commented 2024-09-05 11:24:28 +02:00
Member
Copy Link

I think it's fine to make this function a method on BlendFileBlock. It could then simply be named .clone() or .copy().

Then again, is there any reason to not use copy.copy() and avoid the need for this function altogether?

I think it's fine to make this function a method on `BlendFileBlock`. It could then simply be named `.clone()` or `.copy()`. Then again, is there any reason to not use [copy.copy()](https://docs.python.org/3/library/copy.html#copy.copy) and avoid the need for this function altogether?
Jonas Dichelle commented 2024-09-05 14:27:41 +02:00
Author
Contributor
Copy Link

yes copy should work fine for this too.

yes copy should work fine for this too.
JonasDichelle marked this conversation as resolved
blender_asset_tracer/blendfile/iterators.py Outdated
@ -73,0 +86,4 @@
def dynamic_array(block: BlendFileBlock) -> typing.Iterator[BlendFileBlock]:
"""Generator, yields all blocks in a block that is a dynamic array."""
Sybren A. Stüvel commented 2024-09-05 11:27:35 +02:00
Member
Copy Link

Please add a bit more explanation to this documentation. This first line is fine, but then I would love to see some explanation of what a "dynamic array" is in this context.

Please add a bit more explanation to this documentation. This first line is fine, but then I would love to see some explanation of what a "dynamic array" is in this context.
blender_asset_tracer/blendfile/iterators.py Outdated
@ -73,0 +91,4 @@
for i in range(block.count):
new_block = copy_block(block)
new_block.file_offset = offset
new_block.size = block.dna_type.size // block.count
Sybren A. Stüvel commented 2024-09-05 11:28:46 +02:00
Member
Copy Link

Instead of making Python do the block.dna_type.size lookup twice per loop, do that once outside the loop and store the value in a local variable.

block_size = block.dna_type.size // block.count
Instead of making Python do the `block.dna_type.size` lookup twice per loop, do that once outside the loop and store the value in a local variable. ``` block_size = block.dna_type.size // block.count ```
JonasDichelle marked this conversation as resolved
blender_asset_tracer/blendfile/iterators.py Outdated
@ -73,0 +94,4 @@
new_block.size = block.dna_type.size // block.count
yield new_block
offset += block.dna_type.size
Sybren A. Stüvel commented 2024-09-05 11:29:23 +02:00
Member
Copy Link

Either block.dna_type.size is the total size of the entire array (in which case this needs the // block.count) or it is the size of a single element in the array (in which case the // block.count above is incorrect).

Either `block.dna_type.size` is the total size of the entire array (in which case this needs the `// block.count`) or it is the size of a single element in the array (in which case the `// block.count` above is incorrect).
Jonas Dichelle commented 2024-09-05 13:26:25 +02:00
Author
Contributor
Copy Link

It's the size of an element the division there is incorrect.

It's the size of an element the division there is incorrect.
JonasDichelle marked this conversation as resolved
blender_asset_tracer/trace/modifier_walkers.py
@ -354,0 +371,4 @@
flag = bake.get(b"flag")
flag_bin = bin(flag)
flag_bin_padded = flag_bin[2:].zfill(2)
use_custom_directory = flag_bin_padded[0] == "1"
Sybren A. Stüvel commented 2024-09-05 11:34:16 +02:00
Member
Copy Link

Please don't use string operations to get a single bit flag. Add a constant to cdefs.py with the name of the flag, then use something like:

use_custom_path = bool(flag & cdefs.NODES_MODIFIER_BAKE_CUSTOM_PATH)
Please don't use string operations to get a single bit flag. Add a constant to `cdefs.py` with the name of the flag, then use something like: ```python use_custom_path = bool(flag & cdefs.NODES_MODIFIER_BAKE_CUSTOM_PATH) ```
JonasDichelle marked this conversation as resolved
blender_asset_tracer/trace/modifier_walkers.py
@ -354,0 +382,4 @@
field = mod_directory_field
block = modifier
if directory_ptr == 0:
Sybren A. Stüvel commented 2024-09-05 11:31:49 +02:00
Member
Copy Link

I think it's slightly nicer to not compare to a concrete value, and just use if not directory_ptr:

I think it's slightly nicer to not compare to a concrete value, and just use `if not directory_ptr:`
JonasDichelle marked this conversation as resolved
Jonas Dichelle added 1 commit 2024-09-05 13:26:57 +02:00
Jonas Dichelle added 1 commit 2024-09-05 14:22:14 +02:00
Jonas Dichelle commented 2024-09-05 14:25:19 +02:00
Author
Contributor
Copy Link

Thank you for your feedback!
I made all of the changes you requested and added a unit test for dynamic arrays.
Might be good to add more tests in the future, not sure what would be some good structs to test on though.

Thank you for your feedback! I made all of the changes you requested and added a unit test for dynamic arrays. Might be good to add more tests in the future, not sure what would be some good structs to test on though.
Jonas Dichelle requested review from Sybren A. Stüvel 2024-09-05 14:28:22 +02:00
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u geonodes_support:JonasDichelle-geonodes_support
git checkout JonasDichelle-geonodes_support
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
legacy module
Platforms, Builds, Tests & Devices

  
legacy project
Blender Asset Tracer

  
legacy project
Platform: Windows

  
Priority::Normal

   
Status::Archived

   
Status::Confirmed

   
Status::Needs Triage

   
Status::Resolved

   
Type::Design

   
Type::Report

   
Type::To Do
No Label
legacy module
Platforms, Builds, Tests & Devices
legacy project
Blender Asset Tracer
legacy project
Platform: Windows
Priority::Normal
Status::Archived
Status::Confirmed
Status::Needs Triage
Status::Resolved
Type::Design
Type::Report
Type::To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
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: blender/blender-asset-tracer#92890
No description provided.
Delete Branch "JonasDichelle/blender-asset-tracer:geonodes_support"

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?