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

Conversation

Projects
None yet
8 participants
@obneq
Copy link

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

This comment has been minimized.

Copy link

commented Mar 31, 2015

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

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

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

@nerzhul

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

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

@nerzhul

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Then i follow it with attention :)

@nerzhul nerzhul added the WIP label Apr 1, 2015

@Zeno-

This comment has been minimized.

Copy link
Contributor

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)

This comment has been minimized.

Copy link
@Zeno-

Zeno- Apr 30, 2015

Contributor

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

}
}
irr::scene::E_PARTICLE_AFFECTOR_TYPE getType() const {
return (irr::scene::E_PARTICLE_AFFECTOR_TYPE) 667;

This comment has been minimized.

Copy link
@Zeno-

Zeno- Apr 30, 2015

Contributor

There is no existing enum for whatever 667 is?

v2f texsize
):
scene::ISceneNode(smgr->getRootSceneNode(), smgr)
class fixNumEmitter : public irr::scene::IParticleEmitter {

This comment has been minimized.

Copy link
@Zeno-

Zeno- Apr 30, 2015

Contributor

I think all class names should begin with a capital letter

obneq added some commits Apr 30, 2015

obneq
obneq
obneq
obneq
obneq
@kilbith

This comment has been minimized.

Copy link
Contributor

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-

This comment has been minimized.

Copy link
Contributor

commented May 11, 2015

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

@est31 est31 force-pushed the minetest:master branch to a890c66 Aug 2, 2015

@paramat paramat removed the Medium priority label Mar 5, 2016

@sfan5 sfan5 force-pushed the minetest:master branch to 09f1a0c Dec 21, 2016

@paramat

This comment has been minimized.

Copy link
Member

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.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Jun 11, 2017

Definitively need huge rework, possible close label

@obneq obneq closed this Jun 11, 2017

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.