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 helper #4288
Particle helper #4288
Conversation
We cannot work on this PR as it delete files :). Can you try to build a new one? There are too many files changed |
I understand but I try gulp and gulp typescript what command build all this folder ? |
This seems more related to a promise blemishes with your fork. The dis/preview should not have been deleted |
Better now? |
Can you completely exclude all the dist folder from this PR? |
Seems better now I think |
* @param gpu If the system will use gpu. | ||
* @returns the ParticleSystem created. | ||
*/ | ||
public static Create(type: string, emitter: AbstractMesh, scene: Nullable<Scene> = Engine.LastCreatedScene, gpu: boolean = false): ParticleSystem { |
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 would rather prefer having a json file loaded where you can find fire or smoke parameters
Does it make sense?
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.
Yeah, I understand that, but how can I properly load a JSON file (I already created it in assets/particles)?
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.
Create will be async and you can just call Tools.LoadFile to get the content of the json file (from
baseAssetsUrl + "/systems.json" for instance)
You can rely on promise to get a CreateAsync function for isntance
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.
Oh okay, i haven't see that, thanks you :)
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.
But, can I know why you prefer async method + JSON file over a simple synchronous method?
And I want to do some unit tests but my particle helper does not appear under BABYLON module? Do I have to add this to a config file?
This helps to not bloat the engine core code. |
You're right, thanks. And for the unit tests? |
I have to "gulp typescript" to generate what I've done in order to unit tests ... So after my tests, I have to discard all of the generated dist/preview release for my PR? :o |
You have to discard it before PR |
Yes, but why can't I see a console.log in a unit test to see something? Is the command "gulp tests-unit"? |
It is the right order and it will play all tests and return messages only if there is a problem |
I added the line of my test file in the karma conf and replace the .ts by .js like others files and after I have this error when I launch the tests : 11 05 2018 15:35:08.862:WARN [watcher]: Pattern "/home/devchris/Desktop/Babylon.js/tests/unit/babylon/src/Helpers/babylon.particleHelper.tests.js" does not match any file. |
Where is your ts file located? |
devchris@ubuntu:~/Desktop/Babylon.js/Tools/Gulp$ gulp tests-unit-transpile
[16:49:53] Using gulpfile ~/Desktop/Babylon.js/Tools/Gulp/gulpfile.js
[16:49:53] Starting 'tests-unit-transpile'...
[16:49:56] Finished 'tests-unit-transpile' after 3.03 s
devchris@ubuntu:~/Desktop/Babylon.js/Tools/Gulp$ All good for that, and my file is at : /home/devchris/Desktop/Babylon.js/tests/unit/babylon/src/Helpers/babylon.particleHelper.tests.ts |
Oh, I delete and recreate file and it's work 🤔 |
I don't see the file commited in this PR. Maybe you could commit the assets in a different PR and then you will have the assets online |
Yeah but I have it in local |
I haven't done the tests for now but it seems to work, if this PR get merged, I will do the tests after with the assets endpoint :) |
}); | ||
} | ||
|
||
private static _parseType(type: string): ParticleSystemType { |
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.
not needed
|
||
private static _createSystem(data: IParticleSystemData): ParticleSystem { | ||
// Create a particle system | ||
const fireSystem = new ParticleSystem(data.type, data.capacity, this._scene); |
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.
rename to system
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.
of course!
if (scene) { | ||
this._scene = scene; | ||
} else { | ||
throw new Error("A particle system need a scene."); |
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.
Should be reject() here
@@ -13,6 +13,7 @@ | |||
- Added ability to not generate polynomials harmonics upon prefiltered texture creation ([sebavan](http://www.github.com/sebavan)) | |||
- Added predicate function to customize the list of mesh included in the computation of bounding vectors in the ```getHierarchyBoundingVectors``` method ([sebavan](http://www.github.com/sebavan)) | |||
- Added webVR constructor options: disable laser pointer toggle, teleportation floor meshes ([TrevorDev](https://github.com/TrevorDev)) | |||
- Added a ParticleHelper class to create some pre-configured particle systems in a one-liner method style ([DevChris](https://github.com/yovanoc)) |
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 can promote it to Major updates :)
I will have to do GPU after, but if I replace the return type ParticleSystem by the IParticleSystem interface its telling me that some properties is missing between GPU and normal systems |
Can you tell more about the missing properties? |
minEmitBox, maxEmitBox, direction1, direction2, minAngularSpeed, maxAngularSpeed and all the create...Emitter methods that you tell me to do now |
Ok let's first see the CPU part..we will deal with GPU afterwards |
Yes, you're right, I will push soon :) |
But now all emitters variables in the switch are not used and Travis doesn't like that x) |
system.maxEmitPower = data.maxEmitPower; | ||
system.updateSpeed = data.updateSpeed; | ||
|
||
switch (data.emitterType) { |
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 need for an enum here. Plain string is fine
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.
good like that?
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.
yes!
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.
ahah great
describe('#JSON', () => { | ||
it('creates a fire particle system', () => { | ||
const scene = new BABYLON.Scene(subject); | ||
// |
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.
need to be completed right?
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.
Yes I will do that when assets will be online
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.
all good..just need the unit tests
We will also need detailed doc when everything will be ready |
You are right! I'll do unit tests for now so! :) |
Ping:) Are you working on the documentation? |
Yeah I'm a little bit busy now, I've some work to do, but I will do it :) |
Up:) we really need to get the doc so people can start playing with it |
I've a lot of work but tell me where I have to add the documentation and I will do it :) |
This PR fix #2445.
I don't know if it's good but the dist/preview release folder was deleted, but I tested my ParticleHelper on a separate project after building it with "gulp typescript" and all works great!
In the config.json I am not really sure about the dependUpon things
I haven't done the json part of the config but I will do that soon :)
Here's my first try : https://github.com/yovanoc/Babylon.js/blob/dc4e33cd7f5e9bbddc6cf0966717fdbb89af5022/src/Helpers/babylon.particleHelper.ts