Page MenuHome
Differential D10728

Fix T81957 : Particles inherit object velocity
Needs RevisionPublic
Actions

Authored by Soumya Pochiraju (Forest) on Mar 15 2021, 6:15 PM.

Details

Reviewers
Germano Cavalcante (mano-wii)
Maniphest Tasks
T81957: Particle Systems don't inherit object velocity when emitted on one frame.
Summary

The bug was that particles were not inheriting the object's velocity, because the old function was using particle coordinates to calculate object velocity rather than the object's.

I calculated the objects velocity using FCurves, as suggested in the task description.

Diff Detail

Repository
rB Blender
Branch
particles_inherit_ob_vel (branched from master)
Build Status
Buildable 14255
Build 14255: arc lint + arc unit

Event Timeline

Soumya Pochiraju (Forest) requested review of this revision.Mar 15 2021, 6:15 PM
Soumya Pochiraju (Forest) created this revision.
Soumya Pochiraju (Forest) added a task: T81957: Particle Systems don't inherit object velocity when emitted on one frame. .Mar 15 2021, 7:01 PM
Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).Mar 15 2021, 7:46 PM

https://wiki.blender.org/wiki/Style_Guide
Run make format in the blender folder.

source/blender/blenkernel/intern/particle_system.c
743

Should they be initialised with a sensible default?

750

Space after the first *.
Period after location.

752

This whole block can be a new function.

755

Same as above.

Soumya Pochiraju (Forest) updated this revision to Diff 35235.Mar 16 2021, 4:36 AM

Fixed formatting mistakes in the comments,
Initialised object's current and previous locations

Pratik Borhade (PratikPB2123) retitled this revision from Particles inherit object velocity to Fix T81957 : Particles inherit object velocity.Mar 21 2021, 1:53 AM
Pratik Borhade (PratikPB2123) edited reviewers, added: Nodes & Physics; removed: Physics.
Pratik Borhade (PratikPB2123) set the repository for this revision to rB Blender.
Dalai Felinto (dfelinto) edited reviewers, added: Germano Cavalcante (mano-wii); removed: Nodes & Physics.EditedApr 26 2021, 12:40 PM
Dalai Felinto (dfelinto) added subscribers: Germano Cavalcante (mano-wii), Dalai Felinto (dfelinto).
  1. code-style: you need to run`make format` after your changes.
  2. Please use arcanist to send the patch, so it shows context.
  3. You are changing things that you should not (e.g., dtime != 0.0f -> dtime != 0.0).

The old particles system is on maintaince mode, but since @Germano Cavalcante (mano-wii) seems to have some interest on that (confirmed the bug anyways) I'm setting him as the reviewer.

Soumya Pochiraju (Forest) updated this revision to Diff 36556.Apr 27 2021, 9:03 AM
Soumya Pochiraju (Forest) marked 2 inline comments as done and an inline comment as not done.

Ran make format, fixed the change that shouldn't have been there from previous diff

Harbormaster completed remote builds in B14253: Diff 36556.Apr 27 2021, 9:03 AM
Soumya Pochiraju (Forest) updated this revision to Diff 36557.Apr 27 2021, 9:29 AM

Fixed formatting problems

Harbormaster completed remote builds in B14254: Diff 36557.Apr 27 2021, 9:29 AM
Soumya Pochiraju (Forest) updated this revision to Diff 36558.EditedApr 27 2021, 9:42 AM

created diff after syncing with remote and running rebase, I haven't changed the files from inetrn. Don't know why these formatting differences are showing up

Harbormaster completed remote builds in B14255: Diff 36558.Apr 27 2021, 9:42 AM
Dalai Felinto (dfelinto) added a comment.Apr 28 2021, 9:44 AM

@Soumya Pochiraju (Forest) can you just manually exclude the changes from libmv from your diff then? What may happen is that you may have a different version of clang-format and make format is changing them.

Soumya Pochiraju (Forest) added a comment.Apr 28 2021, 12:42 PM
In D10728#283614, @Dalai Felinto (dfelinto) wrote:

@Soumya Pochiraju (Forest) can you just manually exclude the changes from libmv from your diff then? What may happen is that you may have a different version of clang-format and make format is changing them.

@Dalai Felinto (dfelinto) oh okay, I will do that. But I couldn't find a way to exclude the files. I couldn't find any options at arc diff --help. Could I just copy-paste these segments?

source/blender/blenkernel/intern/particle_system.c
752

where should the function be defined?

Ankit Meel (ankitm) added a comment.Apr 28 2021, 12:50 PM
In D10728#283662, @Soumya Pochiraju (Forest) wrote:
In D10728#283614, @Dalai Felinto (dfelinto) wrote:

@Soumya Pochiraju (Forest) can you just manually exclude the changes from libmv from your diff then? What may happen is that you may have a different version of clang-format and make format is changing them.

@Dalai Felinto (dfelinto) oh okay, I will do that. But I couldn't find a way to exclude the files. I couldn't find any options at arc diff --help. Could I just copy-paste these segments?

Arc diff just uploads the commit. Make sure you stage and commit correctly, and just the changes you want to. Suggest creating a new branch from master and commit right changes in it. Then use arc diff --update D10728

source/blender/blenkernel/intern/particle_system.c
752

A static function just above psys_get_birth_coords should be fine.

Germano Cavalcante (mano-wii) updated this revision to Diff 36787.May 5 2021, 1:02 AM
  • Remove unrelated changes
Harbormaster completed remote builds in B14381: Diff 36787.May 5 2021, 1:02 AM
Germano Cavalcante (mano-wii) requested changes to this revision.May 5 2021, 1:18 AM

It seems like a good idea to recover the object's position through the F-curves but I'm not sure if this is the ideal solution.
It is not just the F-curves that control the object's position (see BKE_object_where_is_calc_time for example).
I think it would be more convenient to somehow save the object's previous position on the particle modifier's own data.

source/blender/blenkernel/intern/particle_system.c
742

A developer venturing into this code may not know quickly that cloc and ploc indicate current location and previous location.
Perhaps a more informative name is: obloc_curr and obloc_prev?
(The lack of information could be compensated with code comments as well).

768

This else is redundant since these values have been set before.

905

When there were no F-curves, sub_v3_v3v3(vel, loc, state->co) always returned a zero vector before?
Because now that is the case.

This revision now requires changes to proceed.May 5 2021, 1:18 AM
Soumya Pochiraju (Forest) added a comment.May 6 2021, 3:12 PM

@Germano Cavalcante (mano-wii) Okay. Thank you for removing the unrelated changes! I will try to implement it by saving the previous location

Soumya Pochiraju (Forest) added inline comments.May 6 2021, 3:15 PM
source/blender/blenkernel/intern/particle_system.c
905

It was previously using current object location and particle location(instead of object's current and previous locations) to compute the value

Revision Contents
Changeset List

PathSize
intern/
libmv/
third_party/
msinttypes/
487 lines
327 lines
source/
blender/
blenkernel/
intern/
31 lines

Diff 36558

View Options

intern/libmv/third_party/msinttypes/inttypes.h

Loading...
View Options

intern/libmv/third_party/msinttypes/stdint.h

Loading...
View Options

source/blender/blenkernel/intern/particle_system.c

Loading...