-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 Sandcastle Examples and Updates #6375
Particle System Sandcastle Examples and Updates #6375
Conversation
@hanbollar, thanks for the pull request! Maintainers, we have a signed CLA from @hanbollar, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
There was a problem hiding this 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 a great start, I have some comments on cleaning up the examples a little, as well as a few visual suggestions.
function startup(Cesium) { | ||
'use strict'; | ||
//Sandcastle_Begin | ||
// Particle Image Grabber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Particle Image Grabber Draw a particle image to a canvas
Just to be super clear what approach you're taking.
shouldAnimate : true | ||
}); | ||
|
||
viewer.scene.debugShowFramesPerSecond = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is more of a visuals demo then a performance demo, I don't think this is needed.
return particleCanvas; | ||
} | ||
|
||
// Cesium Viewer Information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment, it's included in every Sandcastle example.
var cartographicCenterLocation = Cesium.Cartographic.fromCartesian(centerLocation); | ||
var cartographicIncrease = 800.0; | ||
cartographicCenterLocation.height += cartographicIncrease; | ||
centerLocation = Cesium.Cartographic.toCartesian(cartographicCenterLocation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these values? I think there's probably an easier way to accomplish what you are trying to get at here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this Sandcastle example if you're trying to orient something compared to the plane entity.
CHANGES.md
Outdated
@@ -43,6 +45,7 @@ Change Log | |||
* Fixed rendering vector tiles when using `invertClassification`. [#6349](https://github.com/AnalyticalGraphicsInc/cesium/pull/6349) | |||
* Fixed animation for glTF models with missing animation targets. [#6351](https://github.com/AnalyticalGraphicsInc/cesium/pull/6351) | |||
* Fixed double-sided flag for glTF materials with `BLEND` enabled. [#6371](https://github.com/AnalyticalGraphicsInc/cesium/pull/6371) | |||
* Fixed typo in ParticleSystem documentation. [#6375](https://github.com/AnalyticalGraphicsInc/cesium/pull/6375) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really small, I don't think it's worth noting here.
|
||
scene.skyAtmosphere.hueShift = -0.92; | ||
scene.skyAtmosphere.saturationShift = 0.25; | ||
scene.skyAtmosphere.brightnessShift = -0.4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the sky color changes, nice touch.
|
||
#toolbar .header { | ||
font-weight: bold; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, make sure you're using all these styles.
@@ -0,0 +1,186 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you're thumbnail is sized correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sandcastle provides a button to scale the output to the correct thumbnail size. You just need a paint program that can auto-crop to that after a screen capture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resized.
usingOptions.particleSystems.push(item); | ||
} | ||
|
||
function runParticles(usingOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a comment here like "Create several particle systems encircling the base."
shouldAnimate : true | ||
}); | ||
viewer.terrainProvider = new Cesium.CesiumTerrainProvider({ | ||
url : 'https://assets.agi.com/stk-terrain/v1/tilesets/world/tiles' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using the Cesium World Terrain now.
CHANGES.md
Outdated
@@ -33,6 +33,8 @@ Change Log | |||
* Added support for ordering in `DataSourceCollection` [#6316](https://github.com/AnalyticalGraphicsInc/cesium/pull/6316) | |||
* All ground geometry from one `DataSource` will render in front of all ground geometry from another `DataSource` in the same collection with a lower index. | |||
* Use `DataSourceCollection.raise`, `DataSourceCollection.lower`, `DataSourceCollection.raiseToTop` and `DataSourceCollection.lowerToBottom` functions to change the ordering of a `DataSource` in the collection. | |||
* Added more ParticleSystem Sandcastle examples for rocket and comet tails and weather. [#6375](https://github.com/AnalyticalGraphicsInc/cesium/pull/6375) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to 1.45
Can this be accompanied by a short blog post or tutorial with tips on how to setup different effects like this? |
Is this hosted somewhere? Would like to check out the demos but I can't fight the SSL error I am getting right now with this fork. |
That's the plan! PR from @hanbollar to the website incoming.
Not currently, but it can be, see response in #6423 |
318abae
to
5fc1db4
Compare
@ggetz just as a question - i really dont like calling the example for the rocket and comet - Particle System Tails - if you have any better ideas for how to name this pls lmk... i just couldnt think of anything better |
updated all particle system examples to include the proper deprecations. as a note - testing on localhost the visual will seem wrong because dont actually have the deprecations in this branch. waiting on #6429 to actually have the deprecations in master. |
@hanbollar You can merge that branch into this one and change the target of this PR from |
For the naming, "trails" or "streams"? |
@hanbollar Please retarget the base to be the new |
@ggetz resolved conflicts |
remove unused imports
…delete-data Allow Resource.delete to send data
var scratchCartographic = new Cesium.Cartographic(); | ||
var forceFunction = function(options, iteration) { | ||
var iterationOffset = iteration; | ||
var func = function(particle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No abbreviations in variable names, but you should just be able to return the function directly without saving it to a variable.
Source/Core/EllipseGeometry.js
Outdated
@@ -19,11 +19,9 @@ define([ | |||
'./IndexDatatype', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These file changes shouldn't be here. Make sure your branch is up to date with the particle-updates
branch by merging that branch into your instead of master
like normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
particle.velocity = Cesium.Cartesian3.add(particle.velocity, snowGravityScratch, particle.velocity); | ||
|
||
var distance = Cesium.Cartesian3.distance(scene.camera.position, particle.position); | ||
if (distance > (snowRadius)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These parenthesis aren't needed.
particle.position = Cesium.Cartesian3.add(particle.position, rainGravityScratch, particle.position); | ||
|
||
var distance = Cesium.Cartesian3.distance(scene.camera.position, particle.position); | ||
if (distance > (rainRadius)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, unneeded parenthesis.
}); | ||
|
||
// creating particles model matrix | ||
var transl = Cesium.Matrix4.fromTranslation(particlesOffset, new Cesium.Matrix4()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't abbreviate variables.
scratchCartographic = Cesium.Cartographic.fromCartesian(particle.position, Cesium.Ellipsoid.WGS84, scratchCartographic); | ||
|
||
var angle = Cesium.Math.PI * 2.0 * iterationOffset / options.numberOfSystems; | ||
iterationOffset += options.iterationOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we saving
var iterationOffset = iteration;
then increment this new variable, and then not doing anything with it? Unless I'm missing something, I think we can remove that and just use iteration
direction in the above angle calculation.
imageSize : imageSize, | ||
emissionRate : 30.0, | ||
emitter : new Cesium.CircleEmitter(0.1), | ||
bursts : [ ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an empty array, we can just omit it.
I'm still getting deprecation errors when I adjust the sliders in the original particle system demo. Please make sure we are using the new member names, (and update the labels on the UI if needed). |
Thanks @hanbollar, let's finish getting this all cleaned up! |
e9e1248
to
bc7ad08
Compare
@ggetz thanks for the feedback! - updated - lmk if there's anything else |
@hanbollar There are still a lot of extraneous file changes here, please make sure the only changes are your own. Let me know if we need to troubleshoot with git. |
@ggetz yea... i kinda accidentally merged in master first before seeing the note:
i'll work on fixing it |
…ystem-blog-tutorial
emitter : new Cesium.SphereEmitter(rainRadius), | ||
startScale : 1.0, | ||
endScale : 0.0, | ||
image : "../../SampleData/circular_particle.png", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cesium code style - Use single quotes '
, not double quotes "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that previously, update throughout.
|
||
maximumWidth : viewModel.particleSize, | ||
maximumHeight : viewModel.particleSize, | ||
imageSize : new Cesium.Cartesian2(viewModel.particleSize, viewModel.particleSize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the slider for "Size" in the example throws errors. You'll need to have this observable be one number value, then update imageSize
accordingly when update in the following function:
Cesium.knockout.getObservable(viewModel, 'particleSize').subscribe(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@ggetz lmk if u think there's further updates |
scene.fog.minimumBrightness = 0.8; | ||
} | ||
}, { | ||
text : "Rain", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're two text strings should also use single quotes.
* Added color and scale attributes to the `ParticleSystem` class constructor. When defined the variables override startColor and endColor and startScale and endScale. [#6429](https://github.com/AnalyticalGraphicsInc/cesium/pull/6429) | ||
* Improved `MapboxImageryProvider` performance by 300% via `tiles.mapbox.com` subdomain switching. [#6426](https://github.com/AnalyticalGraphicsInc/cesium/issues/6426) | ||
* Added ability to invoke `sampleTerrain` from node.js to enable offline terrain sampling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are above the line - "Added more Particle System Sandcastle examples" - so it's more of just a reordering
particleSystem.imageSize.y = particleSize; | ||
} else { | ||
particleSystem.imageSize = new Cesium.Cartesian2(particleSize, particleSize); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still broken. imageSize
will never be defined- it's not a property of the class. (Additionally you should avoid creating a new Cartesian2 each update).
Replacing the function body with this should work:
particleSystem.minimumImageSize.x = newValue;
particleSystem.minimumImageSize.y = newValue;
particleSystem.maximumImageSize.x = newValue;
particleSystem.maximumImageSize.y = newValue;
11ff9db
to
091558a
Compare
Thanks @hanbollar! Now open a PR from the |
Adding two new sandcastle examples for Particle Systems