Remove correct particle modifier instead of one based on active particle system
ClosedPublic
Actions

Authored by Robert Guetzkow (rjg) on Aug 16 2021, 11:12 PM.

Details

Reviewers

Bastien Montagne (mont29)

Maniphest Tasks

T90715: Removing particle system modifier through Python API, delete wrong modifier

Commits

rB38bdde852f1c: Fix T90715: Remove correct particle modifier through Python API

Summary

This patch fixes T90715. Previously attempting to remove a particle modifier programmatically through Python failed, because it didn't remove the supplied modifier. Instead it deleted the modifier associated with the currently active particle system.

The patch adds an additional argument for the particle system to object_remove_particle_system. This allows to specify which particle system and its associated modifier shall be removed. In case of particle_system_remove_exec it can remain the currently active particle system, whereas object_remove_particle_system can pass the particle system of the modifier.

Diff Detail

Repository

rB Blender

Branch

2021-08-16-modifier-remove (branched from master)

Build Status

Buildable 16453
Build 16453: arc lint + arc unit

Event Timeline

Robert Guetzkow (rjg) requested review of this revision.Aug 16 2021, 11:12 PM

Robert Guetzkow (rjg) created this revision.

Harbormaster completed remote builds in B16453: Diff 40760.Aug 16 2021, 11:12 PM

Robert Guetzkow (rjg) edited the summary of this revision. (Show Details)Aug 16 2021, 11:14 PM

This may need a check in object_remove_particle_system if the supplied modifier is of the correct type. Additionally, the style of the null-checks should be unified. I didn't have time to make these changes yesterday.

Fixed modifier type checks, use consistent syntax for NULL-checks

Harbormaster completed remote builds in B16487: Diff 40810.Aug 17 2021, 10:33 PM

Robert Guetzkow (rjg) retitled this revision from Remove correct particle modifier instead of one based of active particle system to Remove correct particle modifier instead of one based on active particle system.Aug 18 2021, 2:58 PM

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).Aug 19 2021, 9:27 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blenkernel/intern/particle.c
3971	Might it make more sense to take a `ParticleSystem *psys` argument that's never NULL? This way it's always clear which particle system is being removed and the caller is always passing in valid data. This seems a little simpler then passing in a modifier which can be null. If using a modifier argument is preferable, there should be a doc-string here that notes it can be NULL which uses a the active particle system as a fallback.

Robert Guetzkow (rjg) added inline comments.Aug 19 2021, 10:06 AM

source/blender/blenkernel/intern/particle.c
3971	I haven't used that approach because: passing the modifier removes the need to iterate through the modifiers with `psys_get_modifier()`. function like `particle_system_remove_exec()` want the current particle system to be removed. If passing the particle system would require the selection logic to be moved to the function calling `object_remove_particle_system` (through that might be ok). I agree, that passing the particle system directly would simplify the code and the doc-string should be added for the current approach. I'm fine with either approach, if you or @Bastien Montagne (mont29) have a strong preference, then I'll implement that approach.

Campbell Barton (campbellbarton) added inline comments.Aug 19 2021, 10:13 AM

source/blender/blenkernel/intern/particle.c
3971	No worries, also not _that_ opinionated in this case, trust @Bastien Montagne (mont29)'s judgement.

Bastien Montagne (mont29) requested changes to this revision.Aug 20 2021, 3:32 PM

Bastien Montagne (mont29) added inline comments.

source/blender/blenkernel/intern/particle.c
3971	In my comment in chat I actually suggested to pass the `psys` yes, so would rather do that here too. Was only suggesting to make it optional, byut given the number of calls to that function, would not mind making `psys` parameter mandatory. Don't think calls to `psys_get_modifier()` are a real issue in practice (this code is called from operators, not from internal low-level areas executed thousands of times per seconds ;) ). In fact think skipping them in this case is wrong, even if you pass in a particle modifier, you still want to unlink references to its particle system in other existing physics modifiers anyway. So the only call you'd save would be the one to `psys_get_modifier`, not worth it imho.