Page MenuHome

Add support for instancing whole child collections for particles
Needs ReviewPublic

Authored by Elia Sarti (eligt) on Tue, Nov 19, 2:09 AM.

Details

Reviewers
Jacques Lucke (JacquesLucke)
Group Reviewers
Physics
Summary

When selecting Render As: Collection for particles right now you can either render the whole collection as a single instance OR a single object from the collection.

This patch adds support to render the child collections of the specified collection as individual instances for each vertex. This is probably best explained with a picture:

This patch also fixes a behavior when instancing whole collections (and child collections) where the global location of the duplicated objects would be kept as an offset for each particle, forcing users to center the collection at the world origin. I've also adjusted the code so the object list is not allocated when instancing whole collections as this was an unneeded allocation.

Diff Detail

Event Timeline

Jacques Lucke (JacquesLucke) requested changes to this revision.Thu, Nov 21, 10:22 AM

Can "Whole Collection" and "Child Collection" be checked at the same time?
Feels like it would be better to have an enum that would allow the user to choose between different kinds of collection instancing.

Please try to factor out the longer pieces of code into separate functions. The make_duplis_particle_system function is too long already before that.

source/blender/blenkernel/BKE_collection.h
199

What does active mean here?

This revision now requires changes to proceed.Thu, Nov 21, 10:22 AM
Elia Sarti (eligt) added a comment.EditedThu, Nov 21, 2:56 PM

Can "Whole Collection" and "Child Collection" be checked at the same time?
Feels like it would be better to have an enum that would allow the user to choose between different kinds of collection instancing.

When "Whole Collection" is selected "Child Collection" is disabled, similarly to how "Pick Random" is disabled, essentially giving "Whole Collection" higher priority. Adding a flag was the easiest way to add the feature - I did think about the enum, but considering the old particle system is marked as end of life wasn't sure it was worth adding a new enum selection for instantiation type (considering most likely no new types would get added).

Please try to factor out the longer pieces of code into separate functions. The make_duplis_particle_system function is too long already before that.

Personally I don't think it's helpful to break this function, considering that a lot of the code in the function shares many of the same variables. I think there would be no gain in clarity from breaking the function down into smaller ones. The only exceptions being finding locmin and locmax of a list of objects, and accessing a sub-collection by index.

Regarding "active" it was meant to ignore de-activated (i.e. no checkbox) collections but I forgot to check for that in the code, so I'll add it. Edit: seems like this needs the ViewLayer, which makes sense, so I'll just rename the macro and remove active.

but considering the old particle system is marked as end of life wasn't sure it was worth adding a new enum selection for instantiation type

Well, with this logic this new type should not be added as well. Imo, if this is added to the old particle system, then it should be done the "right" way, using an enum. Just because code is marked end-of-life does not mean that we can change it using worse coding standards. All too often, code like this will stay around longer than originally anticipated.

considering that a lot of the code in the function shares many of the same variables

That is in fact one of the main reasons for why functions should be split up...

The only exceptions being finding locmin and locmax of a list of objects, and accessing a sub-collection by index.

Yeah, at least finding the bounding box should be factored out. I'd imagine that there are some utilities for finding bounding boxes already, but maybe not for this specific case.

Added collection render mode rather than Whole Collection and Child Collection checkboxes

I've made the changes to use an enum for rendering collection rather than dual flags and renamed the macro, can you check now if everything is alright?
I don't think splitting the function is worth it for just 10 lines which are only used in one place.

Elia Sarti (eligt) edited the summary of this revision. (Show Details)Sun, Dec 1, 10:53 PM