Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simpler particle birth-offset, and again plus a "_tics_since_birth" getter #769

Conversation

@ArsThaumaturgis
Copy link
Contributor

ArsThaumaturgis commented Nov 1, 2019

As with #766, this pull request is intended to allow one to start a particle effect with a slight offset to particle birthing.

However, where that previous version introduced a new variable, "_initial_birth_offset", this one makes do without. Instead, it modifies the "soft_start" method to take an optional parameter that is used in setting "_tics_since_birth", where before it was being set to "0.0f", if I'm not much mistaken.

The optional parameter has a default value of "0.0f", intended to result in extant code still producing the same effects.

There are then similar changes to related Python classes, allowing this offset to be passed in to the C++ "soft_start" method.

And as before, a "getter" method has been exposed to allow querying of the "_tics_since_birth" variable.

Finally, a simple demonstrative program for these changes:

from direct.showbase.ShowBase import ShowBase
from direct.particles.ParticleEffect import ParticleEffect
from panda3d.core import Vec4

class Game(ShowBase):

    def __init__(self):
        ShowBase.__init__(self)
        self.enableParticles()
        self.win.setClearColor(Vec4(0, 0, 0, 1))

        self.accept("escape", base.userExit)

        self.basicEffect = ParticleEffect()
        self.basicEffect.loadConfig("testParticles.ptf")
        self.basicEffect.setPos(0.75, 5, -0.3)

        self.offsetEffect = ParticleEffect()
        self.offsetEffect.loadConfig("testParticles.ptf")
        self.offsetEffect.setPos(-0.75, 5, -0.3)

        self.basicEffect.start(parent = render, renderParent = render)
        self.offsetEffect.start(parent = render, renderParent = render)
        
        print (self.offsetEffect.getParticlesList()[0].getTicsSinceBirth())

        self.offsetEffect.softStart(firstBirthOffset = -0.25)

        print (self.offsetEffect.getParticlesList()[0].getTicsSinceBirth())

app = Game()
app.run()

"testParticles.ptf" is the same as in the previous version, being is a simple point-particle effect:

self.reset()
self.setPos(0.000, 0.000, 0.000)
self.setHpr(0.000, 0.000, 0.000)
self.setScale(1.000, 1.000, 1.000)
p0 = Particles.Particles('particles-1')
# Particles parameters
p0.setFactory("PointParticleFactory")
p0.setRenderer("PointParticleRenderer")
p0.setEmitter("SphereSurfaceEmitter")
p0.setPoolSize(1024)
p0.setBirthRate(2.0000)
p0.setLitterSize(100)
p0.setLitterSpread(0)
p0.setSystemLifespan(0.0000)
p0.setLocalVelocityFlag(1)
p0.setSystemGrowsOlderFlag(0)
# Factory parameters
p0.factory.setLifespanBase(0.5000)
p0.factory.setLifespanSpread(0.0000)
p0.factory.setMassBase(1.0000)
p0.factory.setMassSpread(0.0000)
p0.factory.setTerminalVelocityBase(400.0000)
p0.factory.setTerminalVelocitySpread(0.0000)
# Point factory parameters
# Renderer parameters
p0.renderer.setAlphaMode(BaseParticleRenderer.PRALPHAOUT)
p0.renderer.setUserAlpha(1.00)
# Point parameters
p0.renderer.setPointSize(1.00)
p0.renderer.setStartColor(Vec4(1.00, 1.00, 1.00, 1.00))
p0.renderer.setEndColor(Vec4(1.00, 1.00, 1.00, 1.00))
p0.renderer.setBlendType(PointParticleRenderer.PPONECOLOR)
p0.renderer.setBlendMethod(BaseParticleRenderer.PPNOBLEND)
# Emitter parameters
p0.emitter.setEmissionType(BaseParticleEmitter.ETRADIATE)
p0.emitter.setAmplitude(1.0000)
p0.emitter.setAmplitudeSpread(0.0000)
p0.emitter.setOffsetForce(Vec3(0.0000, 0.0000, 1.0000))
p0.emitter.setExplicitLaunchVector(Vec3(1.0000, 0.0000, 0.0000))
p0.emitter.setRadiateOrigin(Point3(0.0000, 0.0000, 0.0000))
# Sphere Surface parameters
p0.emitter.setRadius(0.1000)
self.addParticles(p0)

Screenshot from 2019-11-01 21-37-47

…generation--without the addition of a new field, this time.

And, as before, exposure of a method by which to read the "_tics_since_birth" variable.
Copy link
Member

rdb left a comment

Adding a new parameter to a C++ method is not ABI-compatible; you can do it this way, but you need to add it as either a new method entirely or as a new overload of the existing method. (The latter means adding a second version of soft_start that takes two arguments; the right version will be chosen when calling it depending on the number of arguments passed in.)

There's no issue on the Python side with ABI compatibility, though, so that's fine.

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 1, 2019

Ah, I see--fair enough, I'll do that in a little while, then, and update when it's done, I intend! ^_^

…that calls it internally, as well as logic to handle this in "ParticleEffect.py", I believe.
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

All right, I've updated the pull request.

I at first tried to overload the method, but this was complicated by the fact that the extant version of the method has parameter with a default value, meaning that a call without any parameters is valid--and could refer to either of the overloads. Similarly, just creating a version in which one of the parameters has no default, or that only has the offset-parameter, is complicated by the fact that both parameters have the same type.

I've thus added a new method, called "soft_start_offset", which takes the new parameter.

Concomitantly, "ParticleEffect.py" now has a bit of extra logic that allows it to choose between the two versions of "softStart". Perhaps not ideal, but serviceable, I think.

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

PS: With further thought, I think that I actually prefer this approach to adding a variable as in my previous pull request. It feels a little cleaner and neater, and less invasive of other parts of the system, in some ways.

Still, I'll leave that other pull request open for an engine-dev decision on the matter.

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

PPS: Although it has occurred to me that this approach means that such offsets pretty much have to be applied in code, rather than in an editor, if I'm not much mistaken: without a variable, there's no apparent way of saving that a given system should be offset.

Whether that's a significant issue I don't know--after all, particle systems are generally loaded and started in code anyway.

Copy link
Member

rdb left a comment

Looks great! Some points:

  • I don't feel strongly about this, but upon seeing the code, I think I would marginally prefer the second variant to simply be another variant of soft_start rather than to have a different name. This would make the code slightly simpler and make the C++ method mirror the Python method. (For clarification, in C++, there can be multiple versions of the method with the same name, a concept known as "overloading". Our Python bindings automatically select the correct "overload" based on the number of arguments that are passed in. So, assuming you agree with my suggestion, all you need to do is rename the method you created to soft_start, and also remove the default arguments, since otherwise there would be conflict between the two overloads.)

  • I think it's not at all clear what boff means; I get that you are following the example of the even less helpfully named argument called br, but I would prefer if it were called something like offset or birth_delay or whatever you think sounds appropriately documentative in this context. It's probably best to match terminology on the Python version of the method too.

  • It would be good to have the docstring mention this new argument and what it does.

  • Any chance you could toss in a couple of unit tests? After some cough recent experiences I feel really strongly about getting unit tests in for any new changes, no matter how innocent they may seem.

Thanks again for this change! I do think this approach is more elegant than your previous PR. :) It's easier to predict what the behaviour will be if this offset is only specified at start, and if the need arises to make this switchable at any other time, well, we can cross that bridge when we get there.

if self.__isValid():
for particles in self.getParticlesList():
particles.softStart()
if firstBirthOffset is not None:
particles.softStartOffset(boff = firstBirthOffset)

This comment has been minimized.

Copy link
@rdb

rdb Nov 2, 2019

Member

Nitpick: there should not be spaces around a = in keyword argument lists according to PEP8 even though there should be spaces around it almost anywhere else.

This comment has been minimized.

Copy link
@ArsThaumaturgis

ArsThaumaturgis Nov 2, 2019

Author Contributor

Ah, fair enough. My own style includes spaces, but I'll change this to reflect the preferred style for the engine, I intend.

This comment has been minimized.

Copy link
@rdb

rdb Nov 2, 2019

Member

Don't worry too much about this particular issue; we're wildly inconsistent on this particular rule in the Panda codebase. I wouldn't block the PR from merging on this matter. We just try to default to PEP8 since it's the most popular style that is likely to be the most familiar to other developers.

This comment has been minimized.

Copy link
@ArsThaumaturgis

ArsThaumaturgis Nov 2, 2019

Author Contributor

That's fair! But I have other changes to make anyway, as per your other comment, so I might as well include this one, too. ^_^

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

* I don't feel strongly about this, but upon seeing the code, I think I would marginally prefer the second variant to simply be another variant of `soft_start` rather than to have a different name.

I did try (and don't worry, I'm well aware of overloading! ^^; )--but as mentioned above, there was an issue arising from the fact that the extant function has a default value.

As to removing the default value from that function, I'm not sure that I recall all of why I was hesitant to do so.

One point, however, is that the "br" parameter doesn't seem to be much used, within the engine at least. (The Python "softStart" method doesn't use it for one, if I'm not much mistaken.) This means that removing it might, potentially, break extant code.

Hmm... It would, I suppose, be fine for parameterless calls to go through my new method, and thus for the extant method to be without a default value--but if my method has default values for both parameters, then a single-parameter call becomes ambiguous.

Conversely, if my new method has no default values, then there's no path for extant parameterless calls to take...

Right now I don't see a configuration that doesn't potentially cause a problem--aside from just renaming the method.

Believe me, I'd much rather just overload the method than create a renamed method like this! :/

* I think it's not at all clear what `boff` means

Hahah, I fully agree! As you say, I was just trying to match the coding style that I saw in the engine. :P

I much prefer descriptive names, so I'm very happy to rename that parameter if that's acceptable!

* It would be good to have the docstring mention this new argument and what it does.

I take it that that's the comment in the ".i" file, above the method itself? If so, then fair enough, I'll do that I intend! ^_^

(If not, could you point me to the correct place to make this change, please?)

* Any chance you could toss in a couple of unit tests?

I'm honestly not sure of what tests to include. ^^;

It's been a long time since I studied this sort of testing (if I'm correct in recalling that it was covered). A quick look at Wikipedia showed examples that make sense for methods that produce some output--but that's not the case here. I'm not sure of what test would indicate that all was functioning as intended, short of starting a particle system and seeing that it worked... ^^;

So, do you have any suggested tests that I might implement?

Thanks again for this change! I do think this approach is more elegant than your previous PR. :) It's easier to predict what the behaviour will be if this offset is only specified at start, and if the need arises to make this switchable at any other time, well, we can cross that bridge when we get there.

It's my pleasure, and I'm glad that you like this approach! ^_^

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

Ah, I hadn't yet seen your comment in the forum thread. I intend to look into those unit-tests on Monday then, if that's okay!

[edit] (I intend to push the other changes, sans overloading as mentioned above, in the meanwhile however!)

@rdb

This comment has been minimized.

Copy link
Member

rdb commented Nov 2, 2019

If you removed all default arguments from your own overload, then what would happen is this behaviour, which is very much what I think we want:

  • softStart() maps to the original method
  • softStart(1.0) maps to the original method
  • softStart(1.0, 0.5) maps to your method

That does mean that to get an offset without modifying the birth rate, you would need to explicitly pass 0.0 to the first one. On the master branch, we can merge both overloads into one again, so that it will also be possible to call softStart(offset=x) without specifying the first argument.

The docstring is the thing that looks like this:

/**
 * Stuff
 */

Whatever's in that block will get parsed out and turned into our API documentation.

I think I may have answered your question about the unit tests in the Discourse thread. :) Without personally understanding the ParticleSystem API, but just so that you would get an idea, here's what I would imagine it could look like:

# tests/particles/test_particlesystem.py

from panda3d.core import ParticleSystem


def test_system_soft_start():
    system = ParticleSystem(10)
    system.soft_start(1.0)
    assert system.get_tics_since_birth() == 0.0

    # Let 1.5 seconds pass
    system.update(1.5)

    # Assert that the expected number of particles have been birthed
    assert system.get_living_particles() == 1

… and something like that would cover the existing API, while we would have a second unit test that verifies that your variant would indeed affect get_tics_since_birth() result and produce a different birth offset behaviour.

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

If you removed all default arguments from your own overload, then what would happen is this behaviour, which is very much what I think we want:

Ah, I feel silly for not having thought of that--or perhaps I just didn't like the idea of the offset method requiring the "br" parameter; I forget. ^^;

But fair enough--I'll do that then, I intend, especially if they might be merged in the master branch!

The docstring is the thing that looks like this:

Great, I believe that I have the right place, then. ^_^

I think I may have answered your question about the unit tests in the Discourse thread. :)

Indeed, and your example above helps too, I think! Thank you for both answers. ^_^

(I think that I was imagining that the unit tests would be required to produce an immediate answer, rather than being able to run over a few seconds.)

All right; as I said, I'll likely get to unit tests on Monday, if that's okay, but intend to push the other stuff--now including the overload--shortly. (Once I've built and checked on my side (and fixed any errors that doing so might uncover).)

Thaumaturge
 - Changed "soft_start_offset" to be an overload of "soft_start" that has no default values
 - Updated the Python-side soft-start code to reflect the above
  -- Passing in -1 for the value of "br", since the relevant C++ code only checks for a br that is greater than 0, I believe.
 - Made the new parameter to "soft_start" more descriptive than "boff".
 - Updated the doc-string to reflect the offsetting parameter
 - Removed the spaces around the equal-sign in the parameter list for the Python-side softStart method.
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 2, 2019

Right, those changes have been made and committed for review, I believe!

@ArsThaumaturgis ArsThaumaturgis force-pushed the ArsThaumaturgis:ParticleOffsetPlusOne_Simple branch from 447b9e6 to 8bfbc81 Nov 4, 2019
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 4, 2019

Right, I've implemented a unit-test, I believe; please let me know if it's okay!

@rdb

This comment has been minimized.

Copy link
Member

rdb commented Nov 6, 2019

I was about to merge this and just had a thought: do you think it would make more sense if the value were negated, so that a positive value delays the effect (ie. later in time, therefore positive value) and a negative value makes it happen "sooner", or is it more intuitive as-is?

@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 6, 2019

I considered that (indeed, I think that it was the behaviour that I originally envisaged), but I never really came to a decision as to which was preferable.

Thinking about it now:

On the one hand, as it stands it does exactly what it says it does: it offsets the birth-timer by the specified amount.

However, that birth-timer isn't terribly visible to the Python-level user; only by the new "getter"-method can it be retrieved, and it's private even in C++. Thus it seems reasonable to me to guess that most users wouldn't know how the timer works--i.e. does it count up or down?

So, how does one then interpret "offsetting the first birth"? It's honestly a little ambiguous--although I do lean right now towards thinking that it makes the most sense for a positive offset of a birth to result in the birth happening later.

However, it seems to me that if we were to rename the "offset first birth" parameters to something like "delay first birth", then the whole thing would become much clearer: a positive delay converts to a greater duration until birth.

So, perhaps we should both rename the parameters and negate their effect. What do you think?

@rdb

This comment has been minimized.

Copy link
Member

rdb commented Nov 6, 2019

Yes I think I agree with that. A "delay" sounds easier to explain than the idea of "offsetting the counter of the amount of seconds that have passed since the beginning of the effect", and there may (possibly) be more uses for delaying particle effects. We should just be sure to clarify in the docstring that it's OK for the delay to be negative.

…st birth delay", and with that negating the value applied so that it behaves as one is likely to imagine that a delay would, I believe.
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 6, 2019

Okay, that's done, I believe! Take a look and let me know what you think, please.

@rdb
rdb approved these changes Nov 6, 2019
Copy link
Member

rdb left a comment

Looks great (aside from minor nits). Thank you!

I would have preferred the unit test to be separated into two so that it's easier to debug if it fails. If it's testing a separate behaviour then it should probably be separate. But it does not matter much.

if firstBirthOffset is not None:
particles.softStart(br = -1, first_birth_offset_time = firstBirthOffset)
if firstBirthDelay is not None:
particles.softStart(br = -1, first_birth_delay = firstBirthDelay)

This comment has been minimized.

Copy link
@rdb

rdb Nov 6, 2019

Member

No spaces around = when specifying keyword arguments

# us to change the offset-time without affecting the birth-rate.)
system.softStart(br = -1, first_birth_offset_time = 2)
# us to change the delay without affecting the birth-rate.)
system.softStart(br = -1, first_birth_delay = -2)

assert (system.getBirthRate() == 1)
assert (system.getTicsSinceBirth() == 2)

This comment has been minimized.

Copy link
@rdb

rdb Nov 6, 2019

Member

No parentheses around asserts are needed

# again check that the birth-timer has changed as intended,
# and the birth-rate hasn't changed at all.
effect.softStart(firstBirthOffset = -0.25)
effect.softStart(firstBirthDelay = 0.25)

This comment has been minimized.

Copy link
@rdb

rdb Nov 6, 2019

Member

Also spacing around =

Thaumaturge
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 6, 2019

Okay, most of that is done, I believe!

Regarding the coverage of the unit tests, I presume that the two behaviours to which you're referring are the setting of each of the birth-rate and first-birth-delay. If so, then the issue there, I think, is that both behaviours are enacted in the same method. Thus I want to include a check that the birth-rate logic hasn't been affected by the new changes related to the first-birth-delay logic (or, in potential future updates, vice versa).

@rdb
rdb approved these changes Nov 10, 2019
Copy link
Member

rdb left a comment

Great, thanks so much! I mostly meant splitting off the control bit in a separate unit test so that if the control fails, we know there is something else wrong unrelated to this feature. I will do that when merging.

@rdb rdb self-assigned this Nov 10, 2019
@rdb rdb added this to the 1.10.5 milestone Nov 10, 2019
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 10, 2019

Great, thanks so much!

It's my pleasure, and thanks for merging! ^_^

I mostly meant splitting off the control bit ...

Aah, I see! I think that my intent there was to check that the changes made hadn't changed default behaviour--but I can very much see the logic for splitting that section. At the least, there's the argument that the unit tests are intended to have scope beyond this one pull-request, and so shouldn't be designed purely to test it.

Fair enough, then! ^_^

rdb added a commit that referenced this pull request Nov 10, 2019
Co-authored-by: rdb <git@rdb.name>
@rdb rdb closed this in c0daa75 Nov 11, 2019
rdb added a commit that referenced this pull request Nov 11, 2019
@rdb

This comment has been minimized.

Copy link
Member

rdb commented Nov 11, 2019

It's all merged, thanks!

You can already take advantage of this change when deploying your application by putting this in your requirements.txt:

--extra-index-url https://archive.panda3d.org/branches/release/1.10.x
panda3d>=1.10.5.dev92
@ArsThaumaturgis

This comment has been minimized.

Copy link
Contributor Author

ArsThaumaturgis commented Nov 11, 2019

You can already take advantage of this change when deploying your application by putting this in your requirements.txt:

Thank you for that! If I end up completing the new version of my game before 1.10.5 comes out, I may well do so. ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.