Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Particle System #2728
New core particle system. Currently fixing & optimizing & planning.
@emanuele3d On the old PR you said that it was a problem that this doesn't use RenderSystem. It does, the manager implements RendersSystem, and on a render call loops over all of the renderers and passes them the systems that they need to render (in my local version with multiple renderers that is, will push soon.)
Also, I'm not sure how the DAG rendering system works, (a wiki page with at least a tiny explanation would be nice!) But I was wondering on what call we want particles rendered on. Currently everything is on drawAlphaBlended.
Thank you in advance for any explanations.
A couple of quick answers as I can't do a full review right now:
Documentation: high level documentation is problematic at this stage because the system is in a state of flux. It starts to make sense when the DAG-based architecture has stabilized enough to present an API, which is one of the goals of the renderer overhaul. Meanwhile, you might get some high level information in issue #2547. Furthermore, every PR I published of late has added a fair amount of low level documentation, not to mention a focus on self-documenting code - I'm trying to do this for every java file I touch. So, albeit slowly, it's all happening, but it's a long term project and in the short term you'll have to dig into the code.
Regarding the options 1-2 you mention: the "correct" approach is to go through RenderSystems. That's consistent with the current architecture: if you need something to get rendered you should have one or more classes implementing one or more methods of the RenderSystem interface. For example you'd have a set of particles needing alpha blending and those would be rendered through a class implementing the renderAlphaBlend() method. Another set of particles might be fully opaque instead and you'd render them through a system implementing the renderOpaque() method. And in some circumstances the same set of particles might need to be rendered multiple times through multiple methods, all requiring an implementation. The important thing is that particles requiring the same opengl state (i.e. the same shader, face culling enabled/disabled, etc..) should be rendered, as much as possible, within the same RenderSystem.render*() call.
For this purpose I seem to remember you can use the @RegisterSystem decorator or something similar for your instances to self-register. This way the renderer takes care of iterating through your particles calling the methods of the RenderSystem interface at the appropriate time.
New nodes are really only useful to add things that can't be rendered through the existing nodes or that must be done between existing nodes, for whatever reason.
@emanuele3d Thanks for the explanation. I rewrote the multiple renderers scheme to work more like you explained. Here's how you would go about making a custom particle renderer now:
In order for a particle system state data to be sent to a certain particle renderer, you just have to set its renderer.
Overall I think it looks very promising.
What I would see as very important is that it would actually replace the existing particle system.
That way we would avoid having 2 particle systems and it would be good evaluation of this particle system.
Also please then add an explanation of how to test this PR ingame. e.g. mine a block or run the command
spawnPrefab dustEffect once that effect uses the new particle system.
Also I think you could describe the highlights of the PR in the description since I think it has quite a bit to offer :)
referenced this pull request
Jan 22, 2017
I need to interrupt the review here as it's already 2:22am and I better go to sleep.
I will try and finish the review tomorrow or in the next few days.
I added a number of comments to complete the review I started yesterday. In practice I didn't go too in-depth as a addressing yesterday's issues will require significant changes to the code.
After a long discussion with Cervator I have come to the conclusion that it is better for me to get out of the way of this PR. If you submit further PRs on the subject and they are reasonably small I'll make sure to provide feedback.
Bump - am remembering this one is open for merge now. @MaxBorsch could you submit that follow-up issue with details on future improvements needed please, so we don't lose track? Is there any particular easy way to test it? My apologies if that has come up in the past already :-)
Thanks @MaxBorsch :-)
Merged this and tested it out, and it works with a few observations/fixes needed:
I wish I had my mega-workspace so I could detect any other modules with problems (anybody else?) although I suspect the affected count is minor and fixes probably straight forward (could you advise on GooeysQuests, Max?). If we can pair this PR with fixes to affected modules we can let the API violation pass.
Took it for another spin with Terasology/GooeysQuests#13 included - thanks for that :-)
Seemed like it works fine! So that's a good example for how to fix things. Digging around in both my home workspaces I have a clearer picture of what's left on the todo list (some of those I can do while merging)
Ran another quick bit of testing to see if this could be merged in prior to tomorrow's play test even with known module changes to still make, but it crashed pretty hard in multiplayer with GooeysQuests. Immediately on joining player 2 (player 1 hosting) the game crashed with the following error:
GooeysQuests has particles appear when Gooey spawns, which is usually triggered by a player spawning. Player 1 was already in place with a Gooey there, which itself respawned once every 30 seconds or so (unsure of exact timing) causing particles each time.
Also noticing some minor quirks with the particles themselves (like the Gooey special particles seem to have a black background rather than transparency, and some blocks like torches get really weird particles) but those I expect could be ironed out more easily after merging this.
May 6, 2017
1 check passed
added a commit
this pull request
May 6, 2017
Merged this after a crash fix! @MaxBorsch aims to get to the modules soon, so long as we do that before Alpha 8 is released we should be okay. Thanks for all your hard work! :-)
JoshariasSurvival does work in the meantime but will have no particle effects. Some other modules will not until fixed.