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

Particle system updates and name changes #6429

Conversation

hanbollar
Copy link
Contributor

@hanbollar hanbollar commented Apr 5, 2018

includes renaming and adding in deprecations for specific variables and reconfigured methods such as forces is no longer an array, it's just one function callback, and was renamed to updateParticle (etc).

Cleaning up readability of ParticleSystem and Particle classes

TODO:

  • finish all deprecations and deprecation comments
  • once this pull request is done - have another pull request for removing all the deprecated items for the 1.46 release and make sure this is merged in for that release
  • have a third pull request which removes all the start and end and max and min values and just uses an init function s.t. the update function can represent the changes by the user - also allows them to think more openly about what kind of changes they can do - not just force changes but color changes / position based noise changes etc / instead of just force update changes

note: deprecated items to remove:

all legacy variables that match the options. and this. versions of the following:

Particle :
* @param {Cartesian2} [options.size=new Cartesian2(1.0, 1.0)] The dimensions of particles in pixels. This has been deprecated. Use imageSize instead.

ParticleSystem :
* @param {ParticleSystem~applyForce[]} [options.forces] An array of force callbacks. This has been deprecated. Use updateCallback instead.
* @param {Number} [options.rate = 5] The number of particles to emit per second. This has been deprecated. Use emissionRate instead.
* @param {Number} [options.width=1.0] Sets the minimum and maximum width of particles in pixels. This has been deprecated. Use imageSize's x component instead.
* @param {Number} [options.minimumWidth] Sets the minimum width of particles in pixels. This has been deprecated. Use minimumImageSize's x component instead.
* @param {Number} [options.maximumWidth] Sets the maximum width of particles in pixels. This has been deprecated. Use maximumImageSize's x component instead.
* @param {Number} [options.height=1.0] Overwrites the minimum and maximum height of particles in pixels. This has been deprecated. Use imageSize's y component instead.
* @param {Number} [options.minimumHeight] Sets the minimum height of particles in pixels. This has been deprecated. Use minimumImageSize's y component instead.
* @param {Number} [options.maximumHeight] Sets the maximum height of particles in pixels. This has been deprecated. Use maximumImageSize's y component instead.
* @param {Number} [options.lifeTime=Number.MAX_VALUE] How long the particle system will emit particles, in seconds. This has been deprecated. Use lifetime instead.
* @param {Number} [options.life=5.0] If set, overrides the minimumLife and maximumLife inputs with this value. This has been deprecated. Use particleLife instead.
* @param {Number} [options.minimumLife] Sets the minimum life of particles in seconds. This has been deprecated. Use minimumParticleLife instead.
* @param {Number} [options.maximumLife] Sets the maximum life of particles in seconds. This has been deprecated. Use maximumParticleLife instead.

@cesium-concierge
Copy link

Signed CLA is on file.

@hanbollar, thanks for the pull request! Maintainers, we have a signed CLA from @hanbollar, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hanbollar hanbollar force-pushed the particle-system-updates-and-deprecations branch from 8584a08 to 85bcece Compare April 5, 2018 21:02
@@ -329,7 +379,7 @@ define([
}
},
/**
* Sets the maximum speed in meters per second.
* Sets the maximum velocity in meters per second.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needs to be changed back to speed

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

@hanbollar Awesome, thanks!

Let's address the functionality of deprecating the old parameters first, then I'll take a closer look at the code.

CHANGES.md Outdated
* `ParticleSystem.minimumWidth` will no longer be an individual component. Instead of width and height components for pixel dimensions we switched to the Cartesian2, minimumImageSize, instead. Use of the `minimumWidth` parameter is deprecated and will be removed in Cesium 1.46.
* `ParticleSystem.minimumHeight` will no longer be an individual component. Instead of width and height components for pixel dimensions we switched to the Cartesian2, minimumImageSize, instead. Use of the `minimumHeight` parameter is deprecated and will be removed in Cesium 1.46.
* `ParticleSystem.maximumWidth` will no longer be an individual component. Instead of width and height components for pixel dimensions we switched to the Cartesian2, maximumImageSize, instead. Use of the `maximumWidth` parameter is deprecated and will be removed in Cesium 1.46.
* `ParticleSystem.maximumHeight` will no longer be an individual component. Instead of width and height components for pixel dimensions we switched to the Cartesian2, maximumImageSize, instead. Use of the `maximumHeight` parameter is deprecated and will be removed in Cesium 1.46.
Copy link
Contributor

Choose a reason for hiding this comment

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

See if we can combine some of these bullets to be a little more concise, see https://github.com/AnalyticalGraphicsInc/cesium/pull/6429/files#diff-8b1c3fd0d4a6765c16dfd18509182f9dR99

Same for Additions

@@ -40,36 +44,35 @@ define([
*
* @param {Object} [options] Object with the following properties:
* @param {Boolean} [options.show=true] Whether to display the particle system.
* @param {ParticleSystem~applyForce[]} [options.forces] An array of force callbacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll leave this in the docs until it's removed: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#deprecation-and-breaking-changes

But put in the docs that it's deprecated, and tell what to use instead.

(And throughout)

Copy link
Contributor

Choose a reason for hiding this comment

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

The functionality for the old options should still be there for a release, just make sure the warning is thrown when people use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggetz When you say leave it in the docs - do you want the docs to include both the deprecated version and the variable that will take its place or just the original deprecated variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like in this PR (https://github.com/AnalyticalGraphicsInc/cesium/pull/6318/files), we add the warnings, but everything should still work to give developers a chance to update their app. Later, we'll remove the warnings and the old parameters.

Copy link
Contributor Author

@hanbollar hanbollar Apr 6, 2018

Choose a reason for hiding this comment

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

okay - that's for the code - but i'm still confused regarding the documentation itself.

For example in the documentation for the constructor, do we include both the following lines? or just the applyForce[] line?

* @param {ParticleSystem~applyForce[]} [options.forces] An array of force callbacks. This has been deprecated. Use updateCallback instead.

* @param {ParticleSystem~updateCallback} [options.updateCallback] The callback to an update function used for every particle in this system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. When deprecating a function or property, we add the @deprecated tag, but I don't believe you can do that with a parameter, so state it explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

* @param {ParticleBurst[]} [options.bursts] An array of {@link ParticleBurst}, emitting bursts of particles at periodic times.
* @param {Boolean} [options.loop=true] Whether the particle system should loop it's bursts when it is complete.
* @param {Number} [options.speed] Sets the minimum and maximum speed in meters per second
* @param {Number} [options.scale] Sets the start and end scales.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be descriptive of what the option is on its own.

"The scale of the particle throughout it's lifetime"? And maybe mention in start and end that it overrides this value and interpolates between values.

Same with the other values.

Copy link
Contributor Author

@hanbollar hanbollar Apr 6, 2018

Choose a reason for hiding this comment

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

@ggetz I'm not sure I understand why we are making scale the main variable here and letting start and endScale be the secondary variables when scale is not actually used by the class itself anywhere else but in the constructor.

I'm currently describing what the constructor is actually doing with the variable.

Is this change just for readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean just in terms of making the documentation clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would a line like this work for options.scale? [and the same formatting of this line for other start/end and min/max overriding variable documentation]

If set, overrides the startScale and endScale inputs with this value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be best the other way around - When you can set the scale property that's what start and end default to. Then if those are set, they will override that scale value. start and end should not specify default values in the docs. scale should specify the default.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhhh gotcha makes sense now - i'll do that

* @param {Color} [options.startColor=Color.WHITE] The color of a particle when it is born.
* @param {Color} [options.endColor=Color.WHITE] The color of a particle when it dies.
* @param {Object} [options.image] The URI, HTMLImageElement, or HTMLCanvasElement to use for the billboard.
* @param {Cartesian2} [options.imageSize] Sets the maximum and minimum dimensions of the image representing the particle in pixels. Width by Height.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggetz its default is undefined since it's just there to act as a 'double setter' for both minImageSize and maxImageSize.

if it's defined then we actually use it to set minImageSize and maxImageSize and if none of the three are defined then the other two variables are set to their default

Copy link
Contributor

Choose a reason for hiding this comment

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

So if imageSize and minimumImageSize is set, what happens? Which takes precedence over the other? This should be made clear in the docs.

Copy link
Contributor Author

@hanbollar hanbollar Apr 6, 2018

Choose a reason for hiding this comment

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

for clarity, would it be better if I leave minimumImageSize and maximumImageSize as is and just only modify the documentation for imageSize to the following:

If set, overrides the minimumImageSize and maximumImageSize inputs with this value.

* @param {Cartesian2} [options.imageSize] Sets the maximum and minimum dimensions of the image representing the particle in pixels. Width by Height.
* @param {Cartesian2} [options.minimumImageSize=new Cartesian2(1.0, 1.0)] Sets the minimum dimensions of the image representing the particle in pixels. Width by Height.
* @param {Cartesian2} [options.maximumImageSize=new Cartesian2(1.0, 1.0)] Sets the maximum dimensions of the image representing the particle in pixels. Width by Height.
* @param {Number} [options.speed] Sets the minimum and maximum speed in meters per second.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggetz same reasoning for no default for imageSize

* @param {Number} [options.minimumSpeed=1.0] Sets the minimum speed in meters per second.
* @param {Number} [options.maximumSpeed=1.0] Sets the maximum speed in meters per second.
* @param {Number} [options.lifetime=Number.MAX_VALUE] How long the particle system will emit particles, in seconds.
* @param {Number} [options.life] Sets the minimum and maximum life of particles in seconds.
* @param {Number} [options.minimumLife=5.0] Sets the minimum life of particles in seconds.
* @param {Number} [options.maximumLife=5.0] Sets the maximum life of particles in seconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we want to call this particleLife to distinguish it from the system lifetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - will update it

also as a follow up - do you think it'd be a better convention to have both follow the same naming style?
ie - particleLife and systemLife? or did we decide for particleLife and duration or something along those lines? or are we okay with sticking with particleLife and lifetime?

* @param {Number} [options.lifeTime=Number.MAX_VALUE] How long the particle system will emit particles, in seconds.
*
* @demo {@link https://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=ParticleSystem.html|Particle Systems Demo}
* @demo {@link https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Particle%20System.html&label=Showcases|Particle Systems Plane Demo}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call it "Tutorial" instead of Plane Demo? It's not very descriptive.

* @param {Color} [options.startColor=Color.WHITE] The color of a particle when it is born.
* @param {Color} [options.endColor=Color.WHITE] The color of a particle when it dies.
* @param {Object} [options.scale] Sets the start and end scales.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for particle itself, we should just stick with start and end?

Copy link
Contributor Author

@hanbollar hanbollar Apr 6, 2018

Choose a reason for hiding this comment

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

@ggetz hmm i see your point but I also just realized there's no way of knowing a particular particle's current scale and color (the non interpolated ones) do you think it would be a good idea to add actual particle variables and getters for those for user debugging?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say don't, since I think the only use case right now is for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

@@ -26,13 +28,15 @@ define([
* @param {Number} [options.mass=1.0] The mass of particles in kilograms.
Copy link
Contributor

Choose a reason for hiding this comment

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

The mass of particles the particle in kilograms.

hanbollar added 2 commits April 6, 2018 16:43
… current issue is fireworks example is slightly wrong with this implementation. need to figure out why
@hanbollar
Copy link
Contributor Author

@ggetz ready for review

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @hanbollar! This is coming along well, most of my comments are just clarifying the docs.

It would be great if you could open the next PR to remove the deprecated behaviors that we can merge in 1.46 since this is fresh in your head. Can you add a TODO to this PR description so we don't forget?

CHANGES.md Outdated
@@ -11,7 +11,14 @@ Change Log
* Fix Firefox WebGL console warnings. [#5912](https://github.com/AnalyticalGraphicsInc/cesium/issues/5912)
* Fix parsing Cesium.js in older browsers that do not support all TypedArray types. [#6396](https://github.com/AnalyticalGraphicsInc/cesium/pull/6396)

##### Deprecated :hourglass_flowing_sand:
* `Particle.size`, `ParticleSystem.rate`, `ParticleSystem.lifeTime` have been renamed to `Particle.imageSize`, `ParticleSystem.emissionRate`, and `ParticleSystem.lifetime`. Use of the `size`, `rate`, and `lifeTime` parameters is deprecated and will be removed in Cesium 1.46.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to collapse these to one (or two) bullets.

@@ -92,11 +95,15 @@ define([
*/
this.endScale = defaultValue(options.endScale, 1.0);
/**
* The dimensions of the particle in pixels.
* The dimensions of the image representing the particle in pixels. Width by Height.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "The dimensions, width by height, to scale the particle image in pixels."

@@ -40,36 +44,47 @@ define([
*
* @param {Object} [options] Object with the following properties:
* @param {Boolean} [options.show=true] Whether to display the particle system.
* @param {ParticleSystem~applyForce[]} [options.forces] An array of force callbacks.
* @param {ParticleSystem~applyForce[]} [options.forces] An array of force callbacks. This has been deprecated. Use updateCallback instead.
* @param {ParticleSystem~updateCallback} [options.updateCallback] The callback to an update function used for every particle in this system.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "A callback function used to update a particle to be called each frame."

* @param {Number} [options.mass] Sets the minimum and maximum mass of particles in kilograms.
* @param {Number} [options.minimumMass=1.0] Sets the minimum mass of particles in kilograms.
* @param {Number} [options.maximumMass=1.0] Sets the maximum mass of particles in kilograms.
* @param {Number} [options.scale=1.0] Sets the start and end scales.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can clarify a little more the difference between scale and width/height.

Furthermore, I think we should avoid the use of "born" and "dies" and stick to the jargon we use for the rest of the system. How about:

scale: "Sets the scale to apply to the particle image at the start and end of it's life."
startScale: "The initial scale to apply to the particle image of the particle at the start of it's life."
endScale: "The final scale to apply to the particle image of the particle at the end of it's life."

And likewise for the other properties.

* @param {Number} [options.maximumHeight] Sets the maximum height of particles in pixels. This has been deprecated. Use maximumImageSize's y component instead.
* @param {Cartesian2} [options.imageSize=new Cartesian2(1.0, 1.0)] If set, overrides the minimumImageSize and maximumImageSize inputs with this value.
* @param {Cartesian2} [options.minimumImageSize] Sets the minimum dimensions of the image representing the particle in pixels. Width by Height.
* @param {Cartesian2} [options.maximumImageSize] Sets the maximum dimensions of the image representing the particle in pixels. Width by Height.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify that this value is determined at the start of its lifetime and is not interpolated between like scale and color? Likewise for speed, life, and mass?


/**
* A function used to apply a force to the particle on each time step. This has been deprecated because forces[]
* was deprecated. Use updateCallback instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the @deprecated tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

* @param {Number} [options.minimumMass] Sets the minimum mass of particles in kilograms.
* @param {Number} [options.maximumMass] Sets the maximum mass of particles in kilograms.
* @demo {@link https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Particle%20System.html&label=Showcases|Particle Systems Tutorial}
* @demo {@link https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Particle%20System%20Fireworks.html&label=Showcases|Particle Systems Fireworks Demo}
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to the cesiumjs.org Partcile system tutorial as well.

@@ -817,7 +1009,25 @@ define([
};

/**
* A function used to apply a force to the particle on each time step.
* A function used to modify attributes of the particle at each time step. This can include force modifications,
* color, sizing, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can include force modifications, color, sizing, etc.

This is good clarification, but I would remove it and perhaps add a link to the particle tutorial instead for more clarification.

* @callback ParticleSystem~updateCallback
*
* @param {Particle} particle The particle being updated.
* @param {Number} dt The time since the last update.
Copy link
Contributor

Choose a reason for hiding this comment

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

Time in what unit? I assume seconds.

* @param {Number} dt The time since the last update.
*
* @example
* function applyGravity(particle, dt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nitpicky, but since we're not using an array of forces any more, would it make more sense to just call this function updateParticle instead of applyGravity? Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was thinking of leaving it as applyGravity just as a more descriptive example and demoing that they can call the method whatever they wish... However - if there's a name other than applyGravity that you feel works better - such as applyGravityForce or something - pls lmk. I just dont think updateParticle is descriptive enough for the docs.

@hanbollar
Copy link
Contributor Author

@ggetz ready for another look

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @hanbollar !

Just some tweaks. Make sure you run generateDocumentation and make sure all the doc changes here are good to go!

As @hpinkos mentioned, we'll let this sit a few days to make sure anyone else who has feedback on these changes has time to give it. You should be able to go ahead and make your tutorial changes to reflect this.

CHANGES.md Outdated
@@ -11,7 +11,13 @@ Change Log
* Fix Firefox WebGL console warnings. [#5912](https://github.com/AnalyticalGraphicsInc/cesium/issues/5912)
* Fix parsing Cesium.js in older browsers that do not support all TypedArray types. [#6396](https://github.com/AnalyticalGraphicsInc/cesium/pull/6396)

##### Deprecated :hourglass_flowing_sand:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the Deprecated section to below Additions.

* @param {Number} [options.height=1.0] Overwrites the minimum and maximum height of particles in pixels. This has been deprecated. Use imageSize's y component instead.
* @param {Number} [options.minimumHeight] Sets the minimum height of particles in pixels. This has been deprecated. Use minimumImageSize's y component instead.
* @param {Number} [options.maximumHeight] Sets the maximum height of particles in pixels. This has been deprecated. Use maximumImageSize's y component instead.
* @param {Cartesian2} [options.imageSize=new Cartesian2(1.0, 1.0)] If set, overrides the minimumImageSize and maximumImageSize inputs with this value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Still mention this value scales the particle image's dimensions in pixels.

},
set : function(value) {
deprecationWarning('lifeTime', 'lifeTime was deprecated in Cesium 1.45. It will be removed in 1.46. Use lifetime instead.');
this.lifetime.set(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to have this.lifetime = value; to call the setter.

@@ -817,12 +1010,33 @@ define([
};

/**
* A function used to apply a force to the particle on each time step.
* A function used to modify attributes of the particle at each time step. This can include force modifications,
* color, sizing, etc. See the {@link https://cesiumjs.org/tutorials/Particle-Systems-Tutorial/|Particle Systems Tutorial}
Copy link
Contributor

Choose a reason for hiding this comment

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

Try the see tag:

@see {@link https://cesiumjs.org/tutorials/Particle-Systems-Tutorial/|Particle Systems Tutorial}

@hpinkos
Copy link
Contributor

hpinkos commented Apr 12, 2018

I'm a little concerned that all of the property and param renames are just unnecessary breaking changes for our end users. I know I've seen comments before that people get frustrated at the frequency we introduce breaking changes. I'm just worried that the cost of these renames outweighs the benefits. Does anyone else have an opinion on this? @pjcozzi @mramato?

@ggetz
Copy link
Contributor

ggetz commented Apr 12, 2018

What I would argue should definitely stay are the change from an array of forces callbacks to one update callback and the distinction between lifetime and particleLife as that's not particular clear which is the system vs the particle itself (as well the additional doc clarifications made here).

@hanbollar hanbollar force-pushed the particle-system-updates-and-deprecations branch from 31345c8 to 603640c Compare April 12, 2018 20:21
@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 12, 2018

I don't know about the specifics here, but, in general, if the current names are clearly poor and the new names are clearly better, it would be fine to deprecate over 1 to 2 releases since this is likely not a widely used API yet.

However if the renames are only slightly better or just personal preference, please pass on the churn.

@ggetz
Copy link
Contributor

ggetz commented Apr 17, 2018

Thanks @hanbollar, last thing would be to update the Sandcastle example to use the new names. Make sure it doesn't log any warnings.

@hanbollar
Copy link
Contributor Author

hanbollar commented Apr 18, 2018

@ggetz i actually did that in the #6375 branch so that i did all the sandcastle example deprecation updates in one branch [note that that branch is also the one in which i created the new sandcastle examples]

as far as i know the other branch also has no issues as we discussed, so if you're fine with it as well merge this in first then merge in that branch [because i didnt have the 'access' ability to

merge that branch into this one and change the target of this PR from master to that branch.

as mentioned in the other pull request]

also - if there's any issues with that pull request lmk

@ggetz ggetz changed the base branch from master to particle-system-updates April 19, 2018 13:20
@ggetz
Copy link
Contributor

ggetz commented Apr 19, 2018

Thanks @hanbollar. I've created a new branch particle-system-updates we can merge these both into so we can make sure the changes play nicely with each other.

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.

5 participants