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

compatibility with upcoming R72 of three.js #75

Closed
vincent-courtalon opened this issue Aug 31, 2015 · 80 comments

Comments

Projects
None yet
6 participants
@vincent-courtalon
Copy link

commented Aug 31, 2015

Hello, i am not sure if it is really a bug or just a temporary change from R72 dev version of three.js, but using the shaderParticleEngine with it does not work for me (it works with r71 of three.js)
it makes a webgl warning "WARNING: Attribute 0 is disabled. This has signficant performance penalty" on chrome, and the particles are not visible...

if you have any idea of a quick fix (some suggestion i found was using index0AttributeName on material)? I will try later to pinpoint at what commit of three.js it happen.

and thank you for this project, it is a really fast particle engine!

@titansoftime

This comment has been minimized.

Copy link

commented Sep 1, 2015

In the dev version as of this post I get this error: THREE.ShaderMaterial: attributes should now be defined in THREE.BufferGeometry instead.

@titansoftime

This comment has been minimized.

Copy link

commented Sep 15, 2015

@squarefeet r72 is now live. Unfortunately (once again) there are no release notes /sigh.

@weiserhei

This comment has been minimized.

Copy link

commented Sep 16, 2015

Changelog is up 📄 https://github.com/mrdoob/three.js/releases/tag/r72
Also the ones for previous versions, wasnt too problematic to migrate my applications.

I will also update my projects using ShaderParticleEngine shortly and test with r72.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Sep 17, 2015

Hey guys, sorry for the delay in getting back to you all.

Going to grab me some r72 action and see how it fairs; will try to resolve any bugs with it should I (probably) come across any.

Thanks for the heads up :)

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Sep 17, 2015

Okay, first problem I can see is that Shader Material seems to only play ball with THREE.BufferGeometry instances now (rather than plain ol' THREE.Geometry instances, and at least when custom attributes are being used - hence the attribute0 error), so I'll jump on that!

EDIT: @titansoftime - just seen that you'd already pointed this out. Derp.

@titansoftime

This comment has been minimized.

Copy link

commented Sep 17, 2015

@squarefeet Awesome! Super happy to see you on this =]

Your hard work is much appreciated. This library kicks ass.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Sep 17, 2015

@titansoftime Ha, no worries, Walter ;) It may take me a few days to get it up and running (got to convert from Geometry -> BufferGeometry, and don't want to cut any corners), but I'll be playing in the dev branch if you feel like coming along for the ride or want to point out I'm going in a stupid direction.

Currently wondering whether I should store all attribute values in a single ArrayBuffer (per emitter), using Float32Arrays as windows into the buffer, or whether a single buffer-per-attribute would be fine. Not sure of any performance gains from doing the former, nor the latter. Any ideas?

@titansoftime

This comment has been minimized.

Copy link

commented Sep 17, 2015

Ha, no worries, Walter ;)

HA! I don't roll on SHABBOS!!

Not sure of any performance gains from doing the former, nor the latter.

I'm right there with you, not sure. Sorry for my complete uselessness there =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Sep 17, 2015

Saturday, Donny, is Shabbos, the Jewish day of rest. That means that I don't work, I don't drive a car, I don't fucking ride in a car, I don't handle money, I don't turn on the oven, and I sure as shit don't fucking roll!

@titansoftime

This comment has been minimized.

Copy link

commented Sep 17, 2015

"I'm calmer than you are."

@glibersat

This comment has been minimized.

Copy link

commented Oct 5, 2015

thx @squarefeet for having started the port to r72, I can't wait for it to be released! :)

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 5, 2015

@glibersat No worries, sorry it's taking so long - life is getting in the way!

@andrislusis

This comment has been minimized.

Copy link

commented Oct 6, 2015

indeed thx @squarefeet for the effort, I am on the waiting list for the port as well. :)

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Okay, should have some free time coming up this week and over the weekend, so will hopefully have something up next week. Will keep this thread updated with progress! Hang tight, and thanks for the patience!

EDIT: The reason it'll take a bit longer than I expected is that I've got to move over from using THREE.Geometry to THREE.BufferGeometry and as a result swap all the attributes over to THREE.BufferAttributes (which stores data in TypedArrays, leading to incompatibility with THREE.Vector* instances to storing values/props/etc.). Ouch!

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

Ha, yea I started working on this (more so just playing around to better understand the code) and holy crap. Not an easy task. Your efforts are most certainly appreciated =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Ha, thanks. I reckon if I switch over most of the utils functions to work on TypedArrays, that'll make life quite a lot easier, and everything else should fall into place...

Famous last words ;)

What I don't want to do is create a bunch of Vec3s only to use them as storage to place into larger TypedArrays, performance would be terrible; whereas if I work directly on the TypedArray buffer then we might even see a little bit of a speed-up...Maybe! Definitely get a tiny boost from using BufferGeometry instead, that's for sure.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

What I don't want to do is create a bunch of Vec3s only to use them as storage to place into larger TypedArrays

Ha yea that's exactly what I was doing as a test. Scrapped it as a foreseen waste of a time.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

And exactly what I started doing, too!

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

Definitely get a tiny boost from using BufferGeometry instead, that's for sure.

Definitely more efficient memory usage. Should help out on the delay of pushing the data to gpu on initial render. Scenes with dynamically loaded particles (mine for example) should benefit big time.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Yepyep, finger's crossed. The more I think about this port to r72, the more I want to scrap a lot of the library as it is and start not-quite-from-scratch, rather than bastardise what's here already into working how it wasn't designed to! Experiments ahoy...

EDIT: Just to make clear, I'd keep the API as-is; don't want to cause rewrites for people using this library!

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Throwing this out there for opinions:

Would people using this lib be bothered if I slightly changed the way emitter properties were set?

Current way:

var emitter = new SPE.Emitter( {
    sizeStart: 1,
    sizeMiddle: 10,
    sizeEnd: 5,
    // etc.
} );

Potential new way:

var emitter = new SPE.Emitter( {
    size: {
        start: 1,
        middle: 10,
        end: 5
    },
    // etc., same structure for other props such as `opacity` and `angle`
} );

// Or:
var emitter = new SPE.Emitter( {
    size: new SPE.ValueOverLifetime( 1, 10, 5, ... ), // Allows for more than three values
    // etc., same structure for other props such as `opacity` and `angle`
} );

Nothing's set-in-stone yet (as I've not started attacking this bit), so willing to go with general consensus here, and maybe support both options to allow backward compatibility. Happy to stick with existing API.

Do let me know!

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

I like both new methods over the current. A little bit of rewrite is totally worth it IMO.

The first new option looks really clean. The second option looks less clean but more flexible.

I'd have to side with improved functionality over cleaner code.

That's my 2 cents.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

The second option could easily just be an array, actually. Less obvious what it's values are representing, though, I guess.

I'm just rather conscious of causing people to rewrite their code! I'm thinking of nicking ideas from an old v2 of this lib (video) I made a while back but never finished...

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

I'm just rather conscious of causing people to rewrite their code!

I hear ya.

That video is very impressive. Very cool! I love the collision one as well. Mind blowing work man.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Thanks :) The collision was such a PITA. Don't want to revisit that again for a while! I'll see if I can whack CurlNoise support in after I've supported r72.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 7, 2015

The collision was such a PITA

I can only imagine hehe.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2015

Started making decent headway on this. Party's on the dev branch (where all the good parties are) in a temp folder called typedSrc, and is a complete rewrite. Long way to go, but seems to be a good start :)

Edit: Link for the lazy

@vincent-courtalon

This comment has been minimized.

Copy link
Author

commented Oct 8, 2015

Personally, i am ok with both new version, perferably the one with more fonctionnality.
And thanks again for you're work on this library, it is greatly appreciated. I tried to poke in the code, but it still is a little to much for me for now (for my understanding of three.js internals and webgl shaders), Very interesting.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 9, 2015

Cheers for the feedback @vincent-courtalon.

I've hit a massive brick wall, and looks like I'll only be able to support a max of 4 values for valueOverLifetime properties (like color, opacity, size, angle). Stupid GLSL doesn't like non-constants in loops. Might be able to work around it but would need to build shaders dynamically and don't want to go down that route just yet!

Some good news, though: I've got the re-write drawing particles on-screen, and interpolating between various values... Got it up past 2 million particles before I saw a drop below 60fps, so there is a rather nice performance gain so far!

Will try to have a beta out by the end of the weekend, if not shortly after.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 9, 2015

Stupid GLSL doesn't like non-constants in loops.

That is stupid.

Will try to have a beta out by the end of the weekend, if not shortly after.

Hazaa!

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2015

This is why I should've written unit tests first! Will get around to adding those asap.

In the meantime, I've pushed a new update that should fix your issues. Thanks for testing this for me, it's really appreciated :)

@ghost

This comment has been minimized.

Copy link

commented Oct 17, 2015

Why do you use commas instead of semicolons? This is error-prone and confusing as shit.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2015

I like my vars to be declared at the top of functions, lets me know exactly what is defined, rather than having things declared throughout. Never had a problem with accidentally making global vars this way. Not confusing at all.

Potato potato. Tomato tomato.

@ghost

This comment has been minimized.

Copy link

commented Oct 17, 2015

Okay, but

SPE.method1 = function(){
    ...
} , // why not ";" here?
SPE.method2 = function(){
    ...
}
@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2015

Probably because it's still a beta. And semi colon in that scenario isn't necessary, unless followed by parentheses that would execute the function immediately. And that I rewrote this in ~a week in my spare time.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2015

Comma in that scenario is probs a typo, if that's what you're on about.

@vincent-courtalon

This comment has been minimized.

Copy link
Author

commented Oct 17, 2015

this new version look very good, thanks for the work. I have a little bit of time tomorrow so i will try to update my code to use this and i will give you feedback.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 17, 2015

Probably because it's still a beta. And semi colon in that scenario isn't necessary, unless followed by parentheses that would execute the function immediately. And that I rewrote this in ~a week in my spare time.

Yea I noticed those to, Had to fix since I use an auto minify for all my scripts which require syntax perfection unfortunately heh. Not a big deal at all =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2015

Yeah, I think what yisk is on about is commas after function closing brackets, which are typos, probs from a few copypastas pre-refactor. Stuff like that will be sorted out by the time it's released. Exactly what a beta is for - catch things I missed.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 17, 2015

Ok tried current dev build and:

Uncaught ReferenceError: emitter is not defined

SPE.Emitter.prototype._assignAngleValue = function( index ) {
    var array = this.attributes.angle.typedArray,
        prop = emitter.angle, // <---- here
        utils = SPE.utils,
        value;

    if ( utils.arrayValuesAreEqual( prop._value ) && utils.arrayValuesAreEqual( prop._spread ) ) {
        value = utils.randomFloat( prop._value[ 0 ], prop._spread[ 0 ] );
        array.setVec4Components( index, value, value, value, value );
    }
    else {
        array.setVec4Components( index,
            utils.randomFloat( prop._value[ 0 ], prop._spread[ 0 ] ),
            utils.randomFloat( prop._value[ 1 ], prop._spread[ 1 ] ),
            utils.randomFloat( prop._value[ 2 ], prop._spread[ 2 ] ),
            utils.randomFloat( prop._value[ 3 ], prop._spread[ 3 ] )
        );
    }
};
@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 18, 2015

Crap. I need to stop rushing things out. I'll fix it up tomorrow eve. In the meantime, change emitter to this and all will be fine.

Apologies for these silly bugs; I'm going to slow things down a little from now on. Rushing fixes out never ends well, whether beta or not - I don't have time to fully test it Hold fort for the next beta, it'll be a few days; no more hotfixes.

Also in the meantime: big thanks for being testers, it's hugely appreciated, as is everyone's patience in getting r72 support.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 19, 2015

Had more time than I thought I'd have today, so managed to strike quite a few things off the list:

Done

  • Linted src and added strict mode to functions.
  • Removed only existing comma typo in the src code that @yisk pointed out.
  • Added full JSDoc-based documentation (see new docs folder)
  • Added annotated source (again, see docs folder)
  • Added Travis CI (currently only checks dev build. Will add master branch build status once dev is finalised)
  • Added Bower support.
  • Added Browserify support.
  • Added remove() function to SPE.Emitter
  • Ensured that when an emitter is removed from a group, the attribute arrays are spliced accordingly.

Todo

  • Update/create new examples.
  • Create some emitter presets.
  • Add some tutorials/guides.
  • Finish updating the API changelog / migration doc.
  • Final round of testing pre-go live.
@titansoftime

This comment has been minimized.

Copy link

commented Oct 19, 2015

Just tested, working great =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 19, 2015

Fantastico. Will get docs up to scratch and finish testing, then hopefully promote to master soon :)

Btw: Would you find it useful to have a dispose() method on SPE.Group? I see you're having to manually dispose of the material and geometry, and might be helpful (esp. to people that aren't familiar with the internals of this lib) to have a single point of disposal?

@ghost

This comment has been minimized.

Copy link

commented Oct 20, 2015

I really need more examples of complex effects. The library is still far enough from using in production. Let's say I develop a Hearthstone-like CCG for web. How can I reach such effects like a glow around a custom shape? And what about user-defined particle behaviour? Are you consider it to be a bottleneck?

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 20, 2015

Glow around a shape? Like this?

If you could provide more information about what sort of 'complex effects' you are talking about then I'll be able to help you better.

You could always contribute to the library if you feel something is missing.

PS. Please open a new issue for this. I'm not a fan of thread-hijacking.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 20, 2015

Update

Done

  • Testing
  • Added SPE.Group.dispose()
  • Optimised SPE.Emitter.tick(). It's ~15-20% faster now.
  • Optimised rendering. Will only render sections of buffers in use.
  • Fix for when SPE.Emitter.particleCount is 1.

Todo

  • Examples
  • Migration docs
  • Presets (low priority)
  • Finish overview guide.
@titansoftime

This comment has been minimized.

Copy link

commented Oct 21, 2015

Very nice, runs so good =]

EDIT: SPE.Group.dispose() is a welcomed addition, thank you =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2015

Update

Ran as many tests as I could think of and everything looked good so I promoted to master.

r72 support is live!

@squarefeet squarefeet closed this Oct 22, 2015

@titansoftime

This comment has been minimized.

Copy link

commented Oct 23, 2015

@squarefeet I will assume "Optimised rendering. Will only render sections of buffers in use." meant only rendering particles that are from an emitter within view frustum. This in my opinion should be an optional thing for the user. There are cases (which I kinda discovered by accident) where a user would want particles from an out of view emitter to be shown, such as projected fire streams on a wide staircase for example.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 23, 2015

@titansoftime Not quite. Since three.is hates being given larger typed arrays to a buffer attribute than it was given before the scene was rendered (its fine with shrinking them!) I have to allocated memory/buffer sizes upfront. If you specify a larger buffer size (SPE.Group's maxParticles option) than you use then it will only perform draw calls on the sections of the buffers you've used, rather than the whole.

What you're talking about (and you probably know this already!) is fustrum culling. I think it should be fine, but if not let me know. You might have to manually set the fustrumCulled property on the mesh IIRC.

@titansoftime

This comment has been minimized.

Copy link

commented Oct 24, 2015

Ah ok I see.

@vincent-courtalon

This comment has been minimized.

Copy link
Author

commented Oct 27, 2015

just a little followup. I finally managed to get time to update, and it was relatively straightforward (great job!). I must say i like the new way to configure emitters/group.
I will play with the parameters to try to get the same "visual" result as before, but it is already pretty close with a little tweaking. The "wiggle" parameters seem interesting to play with.

as a little bonus, since i just upgraded, i confirm it works fine with the R73 of three, at least with my tests.
thanks for you're work on this library!

@titansoftime

This comment has been minimized.

Copy link

commented Oct 27, 2015

i confirm it works fine with the R73 of three

That's good news =]

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 27, 2015

Tell me about it. I don't want to have to go through that re-write all over again!

Glad that it's working well for you, @vincent-courtalon. I tried to keep the force integration and various other calculations as close to the previous version as I could, so glad you only have to do a few tweaks.

Is r73 released already, then, or still in dev? Did I really take that long doing the re-write?!

By the way, there are a few new bugs opened re. the new rotation option. I'll get around to fixing them as soon as I can.

@vincent-courtalon

This comment has been minimized.

Copy link
Author

commented Oct 27, 2015

r73 is released, it is on the master branch.

Honestly i was surprised, i was planning to use r72, but when i checked on github, saw that r73 was just released a few days ago, so i thought that i might as well update directly to this version and see if it works.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 27, 2015

Wow, they don't hang around, do they? I'd best check the changelog...

@titansoftime

This comment has been minimized.

Copy link

commented Oct 27, 2015

Honestly i was surprised

Same here.

@squarefeet

This comment has been minimized.

Copy link
Owner

commented Oct 27, 2015

The only thing that stands out to me is this, on WebGLRenderer:

  • Unified drawRange and group ranges.

I'll have to make sure it's still working as it was in r72 (I'm assuming so...)

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