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

wip irrlicht particles 2 #2587

Closed
wants to merge 17 commits into from
Closed

wip irrlicht particles 2 #2587

wants to merge 17 commits into from

Conversation

obneq
Copy link

@obneq obneq commented Mar 31, 2015

next attempt here, using custome scene node:
https://github.com/obneq/minetest/tree/scenenodeparticles

sorry for closing and reopening instead of updating PR. this is attempt 2 at a naive replacement of mt particles with irrlicht ones.
it does not yet look like current mt particles and is not meant for straight merge. Instead, developers please review and give your feedback! Please note that this is very much WIP. Ignore style and minor issues if you can and instead let me know what could be done better differently in a larger sense.

@C1ffisme
Copy link

What advantages does it pose? Does it make them faster/better?

@kwolekr
Copy link
Contributor

kwolekr commented Apr 1, 2015

Both faster and better. This is using the builtin Irrlicht particle functionality which we've been waiting for :)

@nerzhul
Copy link
Member

nerzhul commented Apr 1, 2015

The original pr has collision bugs and generate some lag on Intel GPU

@nerzhul
Copy link
Member

nerzhul commented Apr 1, 2015

Then i follow it with attention :)

@nerzhul nerzhul added the WIP The PR is still being worked on by its author and not ready yet. label Apr 1, 2015
@Zeno-
Copy link
Contributor

Zeno- commented Apr 30, 2015

I'm not sure how I missed this PR. Watching.

float pps = event->add_particlespawner.amount;
float time = event->add_particlespawner.spawntime;

if (time != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't compare floating point numbers to an absolute number

@kilbith
Copy link
Contributor

kilbith commented May 11, 2015

So I tested that PR with your latest commit (4470c27) and here's my report :

1. Compiler's warnings
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const vector3df& FixNumEmitter::getDirection() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:108:87: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const core::vector3df& getDirection() const { return v3f(0.0, 0.0, 0.0); }
                                                                                       ^
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const irr::video::SColor& FixNumEmitter::getMinStartColor() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:111:87: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const video::SColor& getMinStartColor() const { return video::SColor(1); }
                                                                                       ^
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const irr::video::SColor& FixNumEmitter::getMaxStartColor() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:112:87: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const video::SColor& getMaxStartColor() const { return video::SColor(1);; }
                                                                                       ^
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const dimension2df& FixNumEmitter::getMaxStartSize() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:113:107: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const core::dimension2df& getMaxStartSize() const { return core::dimension2d<f32>(0.2, 0.2); }
                                                                                                           ^
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const dimension2df& FixNumEmitter::getMinStartSize() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:114:107: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const core::dimension2df& getMinStartSize() const { return core::dimension2d<f32>(0.2, 0.2); }
                                                                                                           ^
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp: In member function ‘virtual const vector3df& FixNumEmitter::getCenter() const’:
/home/jp/git/irrlicht-particles/minetest/src/particles.cpp:115:84: warning: returning reference to temporary [-Wreturn-local-addr]
         virtual const core::vector3df& getCenter() const { return v3f(0.0, 0.0, 0.0); }
                                                                                    ^
2. In-game comparison

Particles on current master (5a1975a) - they have the good direction.
screenshot_20150511_150136

Same configuration with your Irrlicht particles branch - particles are spreading randomly everywhere surrounded by a moving white box - performances seems similar to current master.
screenshot_2015-05-11t15 00 30
screenshot_2015-05-11t15 15 56

@Zeno-
Copy link
Contributor

Zeno- commented May 11, 2015

Yes, well. Those warnings cannot be disregarded at all.

@paramat
Copy link
Contributor

paramat commented Jan 3, 2017

@obneq perhaps someone could adopt this work and continue it? Or are you planning to restart work? MT particles really need improvement.

@paramat paramat added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Apr 29, 2017
@nerzhul
Copy link
Member

nerzhul commented Jun 11, 2017

Definitively need huge rework, possible close label

@obneq obneq closed this Jun 11, 2017
@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Possible close WIP The PR is still being worked on by its author and not ready yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants