Skip to content

10 randomise selected properties per frame#35

Merged
sfmig merged 18 commits intomainfrom
10-randomise-selected-properties-per-frame
May 12, 2023
Merged

10 randomise selected properties per frame#35
sfmig merged 18 commits intomainfrom
10-randomise-selected-properties-per-frame

Conversation

@ruaridhg
Copy link
Copy Markdown
Collaborator

@ruaridhg ruaridhg commented Apr 21, 2023

When frame is changed apply_random_transform operator is called

@ruaridhg ruaridhg linked an issue Apr 21, 2023 that may be closed by this pull request
3 tasks
Copy link
Copy Markdown
Collaborator

@sfmig sfmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great!

Since it was easy to implement, I'd suggest we add it to the materials panel in this same PR. Let me know thoughts.

Comment thread blender_randomiser/randomiser/transform/operators.py Outdated
Comment thread blender_randomiser/randomiser/transform/operators.py Outdated
Comment thread blender_randomiser/randomiser/transform/operators.py Outdated
Comment thread blender_randomiser/randomiser/transform/operators.py Outdated
Comment thread blender_randomiser/randomiser/transform/operators.py Outdated
@ruaridhg ruaridhg requested a review from sfmig April 25, 2023 15:25
Copy link
Copy Markdown
Collaborator

@sfmig sfmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not randomising the material properties for me (was it for you?) - EDIT: I fixed it here

I also get this error:

rna_uiItemO: operator missing srna 'opr.apply_random_transform'
/Users/sofia/Library/Application Support/Blender/3.4/scripts/addons/randomiser/transform/ui.py:45

I think because it is actually using the operator defined in transforms.py (hopefully fixed here).

Can we delete that script to avoid confusion/error?



@persistent
def randomise_per_frame(dummy):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect you need to call the functions with different names, for the transforms operator and the materials operator? Otherwise I suspect they'd be overwritten right? (that's why I suggested calling this randomise_camera_transform_per_frame earlier)

although that doesn't seem to fix the issue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it here

Copy link
Copy Markdown
Collaborator Author

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this now?

Copy link
Copy Markdown
Collaborator Author

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this now?

Copy link
Copy Markdown
Collaborator Author

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ruaridhg ruaridhg requested review from sfmig and tdowrick May 10, 2023 10:15
@ruaridhg
Copy link
Copy Markdown
Collaborator Author

I've managed to sort out the merge conflicts now and include the update main with the global seed working properly so should be fine to merge now.

@sfmig
Copy link
Copy Markdown
Collaborator

sfmig commented May 12, 2023

Sorry @ruaridhg I missed the comments in this PR!

There was a small issue with the operator name for the material nodes (merging artefact?), but I fixed it.

Another thing I noticed is that if the seed is set, the sequence of random numbers will be deterministic, but we are still randomising the values every time a frame change is executed. This means that if we have some values in frame 1, then switch to another frame, and then go back to 1, the values won't be the same. So the random values are not linked to a frame number. The result is that if we want to reproduce a specific sequence of numbers, we also need to register the frame transitions.

I am happy to merge for now as I think maybe it is fine as it is, but maybe we need to check with @tdowrick if having values linked to specific frames is the intended behaviour.

@sfmig sfmig merged commit 3256b62 into main May 12, 2023
@sfmig
Copy link
Copy Markdown
Collaborator

sfmig commented May 12, 2023

actually the issue above may be related to the latest seed PR not being already merged... will keep an eye on it 🤔

@ruaridhg ruaridhg deleted the 10-randomise-selected-properties-per-frame branch September 20, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Randomise selected properties per frame

2 participants