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

[CSM] Add local particles and particlespawners. #5492

Closed
wants to merge 1 commit into from

Conversation

@red-001
Copy link
Contributor

commented Mar 31, 2017

  • Add core.add_particle
  • Add core.add_particlespawner
  • Add core.delete_particlespawner
  • Document new API
  • Fix code style
  • Test if everything works

Part of #5394
You can't attach the particle spawner to objects yet as there is no objectref on the client.

@red-001 red-001 referenced this pull request Mar 31, 2017

Closed

[CSM] Second Roadmap #5394

15 of 27 tasks complete

u8 glow = 0;

if (lua_istable(L, 1)) {

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 1, 2017

Member

should just abort if it isn't a table

@red-001 red-001 force-pushed the red-001:csm_particles branch 2 times, most recently from 31f38d3 to eff6596 Apr 1, 2017

void ParticleManager::stepSpawnersLocal (float dtime)
{
MutexAutoLock lock(m_spawner_local_list_lock);
for (std::map<u32, ParticleSpawner*>::iterator i =

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 1, 2017

Member

i think you should use a UNORDERED_MAP for this

@@ -455,6 +456,26 @@ void ParticleManager::stepSpawners (float dtime)
}
}

void ParticleManager::stepSpawnersLocal (float dtime)
{
MutexAutoLock lock(m_spawner_local_list_lock);

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 1, 2017

Member

why a mutex ? i don't see any other thread using it

This comment has been minimized.

Copy link
@red-001

red-001 Apr 1, 2017

Author Contributor

I just duplicated the way that the list of particlespawners sent by the server is handled.

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 6, 2017

Member

okay this should be removed as client doesn't have locks here

This comment has been minimized.

Copy link
@red-001

red-001 Apr 7, 2017

Author Contributor

done

@red-001 red-001 changed the title [CSM] Start work on csm particles. [CSM] Add local particles and particlespawners. Apr 1, 2017

@red-001 red-001 force-pushed the red-001:csm_particles branch 2 times, most recently from 37e8dd5 to f496806 Apr 1, 2017

@@ -433,6 +430,7 @@ void ParticleManager::step(float dtime)
{
stepParticles (dtime);
stepSpawners (dtime);
stepSpawnersLocal (dtime);

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

stepSpawnersLocal(dtime); (without space)

{
i->second->step(dtime, m_env);
++i;
}

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

Needs code style fixes.

for(UNORDERED_MAP<u32, ParticleSpawner*>::iterator i =
m_particle_spawners_local.begin();
i != m_particle_spawners_local.end();)
{

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

for (<snip>) {

m_particle_spawners_local.erase(event->delete_particlespawner.id);
}
// no allocated memory in delete event
break;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

-2 indents for the whole block

// vertical = bool
// texture = e.g."default_wood.png"
// animation = TileAnimation definition
// glow = num

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

Types should be embedded into line 28-30 or removed entirely (we have the documentation in lua_api.txt)

lua_pop(L, 1);

lua_getfield(L, 1, "velocity");
vel = lua_istable(L, -1) ? check_v3f(L, -1) : vel;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

Inconsistent with pos.

// vertical = bool
// texture = e.g."default_wood.png"
// animation = TileAnimation definition
// glow = num

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

Types here again.

v3f minpos, maxpos, minvel, maxvel, minacc, maxacc;
minpos = maxpos = minvel = maxvel = minacc = maxacc = v3f(0, 0, 0);
float time, minexptime, maxexptime, minsize, maxsize;
time = minexptime = maxexptime = minsize = maxsize = 1;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

IMO, this would look better if the default values were set in the conditional expressions below.

minsize = getfloatfield_default(L, 1, "minsize", minsize);
maxsize = getfloatfield_default(L, 1, "maxsize", maxsize);
collisiondetection =
getboolfield_default(L, 1,"collisiondetection", collisiondetection);

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 2, 2017

Member

1x Space missing

u32 ParticleManager::getSpawnerId()
{
u32 id = 0;
for (;;) { // look for unused particlespawner id

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Apr 2, 2017

Member
for (u32 id = 0;; ++id) {
@@ -0,0 +1,205 @@
/*

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Apr 2, 2017

Member

is it not possible to reuse the server's particles? Ie: by adding a member variable to indicate is_local

This comment has been minimized.

Copy link
@red-001

red-001 Apr 3, 2017

Author Contributor

I could but that file includes a lot of backwards compatibility related stuff that isn't really wanted in a new api.

@red-001 red-001 force-pushed the red-001:csm_particles branch 2 times, most recently from 6c61d93 to b22ce8f Apr 4, 2017

@sofar

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

Tested. Getting easily 1k+ particles client side. Very nice.

👍

@nerzhul nerzhul modified the milestone: 0.4.16 Apr 6, 2017

@Fixer-007 Fixer-007 referenced this pull request Apr 6, 2017

Closed

Minetest 0.4.16 roadmap #5530

@red-001 red-001 force-pushed the red-001:csm_particles branch from b22ce8f to c66cd10 Apr 7, 2017

@bigfoot547

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2017

@sofar Is that an approval?

@@ -511,6 +527,15 @@ void ParticleManager::handleParticleEvent(ClientEvent *event, Client *client,
// no allocated memory in delete event
break;
}
case CE_DELETE_LOCAL_PARTICLESPAWNER: {
if (m_particle_spawners_local.find(event->delete_particlespawner.id) !=

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 8, 2017

Member
UNORDERED_MAP<u32, ParticleSpawner*>::iterator it =
	m_particle_spawners_local.find(event->delete_particlespawner.id);
if (it != m_particle_spawners_local.end()) {
	delete it->second;
	m_particle_spawners_local.erase(it);
}

Makes the whole thing easier to read (yes, it's done this way aswell below).
EDIT: same for case CE_ADD_LOCAL_PARTICLESPAWNER:

i != m_particle_spawners_local.end();) {
delete i->second;
m_particle_spawners_local.erase(i++);
}

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 8, 2017

Member
for (UNORDERED_MAP<u32, ParticleSpawner*>::iterator it =
		m_particle_spawners_local.begin();
		it != m_particle_spawners_local.end(); ++it) {
	delete it->second;
	m_particle_spawners_local.erase(it);
}

lgtm answers from @red-001

@red-001 red-001 force-pushed the red-001:csm_particles branch from 79a5d24 to 203847f Apr 29, 2017

Changes were made.

}

### `ParticleSpawner` definition (`add_particlespawner`)
**NOTE: `attached` is not implemented yet.**

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

then remove it from here?

delete i->second;
m_particle_spawners.erase(i++);
m_particle_spawners.erase(i);

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

are you sure this works?
erasing & iterating at the same time needs some special care

i != m_particle_spawners_local.end();) {
if (i->second->get_expired()) {
delete i->second;
m_particle_spawners_local.erase(i++);

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

now why is this different from stepSpawners?

stepSpawners (dtime);
stepParticles(dtime);
stepSpawners(dtime);
stepSpawnersLocal(dtime);
}

void ParticleManager::stepSpawners (float dtime)

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

space could be removed here too

delete i->second;
m_particle_spawners.erase(i++);
m_particle_spawners.erase(i);

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

have you tested this

}
break;
// no allocated memory in delete event
break;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

double break

delete it->second;
m_particle_spawners_local.erase(it);
}
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

why is this in { }'s?

delete event->add_particlespawner.texture;
delete event->add_particlespawner.maxacc;

{

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

same

m_particle_spawners_local.find(id);
if (f == m_particle_spawners_local.end()) {
return id;
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 29, 2017

Member

brace removal possible

@sfan5 sfan5 removed the Rebase needed label Apr 29, 2017

@nerzhul nerzhul added this to To Do in Minetest 0.4.16 Apr 29, 2017

@sofar

This comment has been minimized.

Copy link
Member

commented May 4, 2017

doesn't compile:

CMake Error at src/CMakeLists.txt:575 (add_executable):
  Cannot find source file:

    /home/sofar/git/minetest/src/script/lua_api/l_particles_local.cpp

@red-001 red-001 force-pushed the red-001:csm_particles branch 2 times, most recently from b82c8ce to d00e0e2 May 6, 2017

@nerzhul nerzhul moved this from To Do to Requests in Minetest 0.4.16 May 21, 2017

@nerzhul nerzhul removed the One approval label May 21, 2017

@nerzhul nerzhul removed this from the 0.4.16 milestone May 21, 2017

@nerzhul nerzhul removed this from Requests in Minetest 0.4.16 May 21, 2017

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

closing this as I'm working on a better way to implement this.

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.