[CSM] Add local particles and particlespawners. #5492

red-001 · 2017-03-31T19:36:36Z

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

sfan5 · 2017-04-01T09:38:31Z

src/script/lua_api/l_particles_local.cpp

+
+	u8 glow = 0;
+
+ if (lua_istable(L, 1)) {


should just abort if it isn't a table

nerzhul · 2017-04-01T12:06:41Z

src/particles.cpp

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


i think you should use a UNORDERED_MAP for this

nerzhul · 2017-04-01T12:07:37Z

src/particles.cpp

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

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


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

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

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

SmallJoker · 2017-04-02T14:16:51Z

src/particles.cpp

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


stepSpawnersLocal(dtime); (without space)

SmallJoker · 2017-04-02T14:17:06Z

src/particles.cpp

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


Needs code style fixes.

SmallJoker · 2017-04-02T14:17:47Z

src/particles.cpp

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


for (<snip>) {

SmallJoker · 2017-04-02T14:18:27Z

src/particles.cpp

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


-2 indents for the whole block

SmallJoker · 2017-04-02T14:24:48Z

src/script/lua_api/l_particles_local.cpp

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


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

SmallJoker · 2017-04-02T14:28:45Z

src/script/lua_api/l_particles_local.cpp

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


Inconsistent with pos.

SmallJoker · 2017-04-02T14:30:10Z

src/script/lua_api/l_particles_local.cpp

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


Types here again.

SmallJoker · 2017-04-02T14:31:39Z

src/script/lua_api/l_particles_local.cpp

+	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;


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

SmallJoker · 2017-04-02T14:31:54Z

src/script/lua_api/l_particles_local.cpp

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


1x Space missing

rubenwardy · 2017-04-02T23:23:44Z

src/particles.cpp

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


for (u32 id = 0;; ++id) {

rubenwardy · 2017-04-02T23:24:40Z

src/script/lua_api/l_particles_local.cpp

@@ -0,0 +1,205 @@
+/*


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

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

sofar · 2017-04-06T05:42:56Z

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

👍

bigfoot547 · 2017-04-08T02:38:50Z

@sofar Is that an approval?

SmallJoker · 2017-04-08T14:38:31Z

src/particles.cpp

@@ -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) !=


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:

SmallJoker · 2017-04-08T14:41:27Z

src/particles.cpp

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


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); }

@red-001

lgtm answers from @red-001

Changes were made.

sfan5 · 2017-04-29T13:51:08Z

doc/client_lua_api.md

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


then remove it from here?

sfan5 · 2017-04-29T13:52:12Z

src/particles.cpp

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


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

sfan5 · 2017-04-29T13:52:36Z

src/particles.cpp

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


now why is this different from stepSpawners?

sfan5 · 2017-04-29T13:52:45Z

src/particles.cpp

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

 void ParticleManager::stepSpawners (float dtime)


space could be removed here too

sfan5 · 2017-04-29T13:52:56Z

src/particles.cpp

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


have you tested this

sfan5 · 2017-04-29T13:54:08Z

src/particles.cpp

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


double break

sfan5 · 2017-04-29T13:54:43Z

src/particles.cpp

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


why is this in { }'s?

sfan5 · 2017-04-29T13:54:48Z

src/particles.cpp

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


sfan5 · 2017-04-29T13:55:01Z

src/particles.cpp

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


brace removal possible

sofar · 2017-05-04T04:16:26Z

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 · 2017-06-28T17:07:46Z

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

nerzhul added the @ Client Script API label Mar 31, 2017

red-001 mentioned this pull request Mar 31, 2017

[CSM] Second Roadmap #5394

Closed

27 tasks

sfan5 reviewed Apr 1, 2017

View reviewed changes

src/script/lua_api/l_particles_local.cpp Outdated

u8 glow = 0;

if (lua_istable(L, 1)) {

Copy link

Member

sfan5 Apr 1, 2017

Choose a reason for hiding this comment

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

should just abort if it isn't a table

red-001 force-pushed the csm_particles branch 2 times, most recently from 31f38d3 to eff6596 Compare April 1, 2017 12:03

nerzhul reviewed Apr 1, 2017

View reviewed changes

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

red-001 force-pushed the csm_particles branch 2 times, most recently from 37e8dd5 to f496806 Compare April 2, 2017 07:26

SmallJoker reviewed Apr 2, 2017

View reviewed changes

src/particles.cpp Outdated

{

i->second->step(dtime, m_env);

++i;

}

Copy link

Member

SmallJoker Apr 2, 2017

Choose a reason for hiding this comment

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

Needs code style fixes.

SmallJoker reviewed Apr 2, 2017

View reviewed changes

rubenwardy previously requested changes Apr 2, 2017

View reviewed changes

red-001 force-pushed the csm_particles branch 2 times, most recently from 6c61d93 to b22ce8f Compare April 4, 2017 17:30

nerzhul modified the milestone: 0.4.16 Apr 6, 2017

Fixer-007 mentioned this pull request Apr 6, 2017

Minetest 0.4.16 roadmap #5530

Closed

red-001 force-pushed the csm_particles branch from b22ce8f to c66cd10 Compare April 7, 2017 16:37

SmallJoker reviewed Apr 8, 2017

View reviewed changes

nerzhul added the Rebase needed The PR needs to be rebased by its author. label Apr 27, 2017

nerzhul added this to In Progress in Server-sent Client Side Modding Apr 29, 2017

red-001 force-pushed the csm_particles branch from 79a5d24 to 203847f Compare April 29, 2017 13:46

sfan5 reviewed Apr 29, 2017

View reviewed changes

src/particles.cpp

delete i->second;

m_particle_spawners.erase(i++);

m_particle_spawners.erase(i);

Copy link

Member

sfan5 Apr 29, 2017

Choose a reason for hiding this comment

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

have you tested this

sfan5 reviewed Apr 29, 2017

View reviewed changes

src/particles.cpp Outdated

}

break;

// no allocated memory in delete event

break;

Copy link

Member

sfan5 Apr 29, 2017

Choose a reason for hiding this comment

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

double break

sfan5 reviewed Apr 29, 2017

View reviewed changes

src/particles.cpp Outdated

delete it->second;

m_particle_spawners_local.erase(it);

}

}

Copy link

Member

sfan5 Apr 29, 2017

Choose a reason for hiding this comment

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

why is this in { }'s?

sfan5 reviewed Apr 29, 2017

View reviewed changes

src/particles.cpp Outdated

delete event->add_particlespawner.texture;

delete event->add_particlespawner.maxacc;

{

Copy link

Member

sfan5 Apr 29, 2017

Choose a reason for hiding this comment

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

same

sfan5 reviewed Apr 29, 2017

View reviewed changes

sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Apr 29, 2017

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

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

[CSM] Add local particles and particlespawners.

d00e0e2

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

nerzhul removed the One approval ✅ ◻️ label May 21, 2017

nerzhul removed this from the 0.4.16 milestone May 21, 2017

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

SmallJoker added Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author. labels Jun 17, 2017

red-001 closed this Jun 28, 2017

red-001 mentioned this pull request Jun 28, 2017

[CSM] Add functions to create particles and particlespawners. #6072

Merged

nerzhul moved this from In Progress to Aborted in Server-sent Client Side Modding Jul 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CSM] Add local particles and particlespawners. #5492

[CSM] Add local particles and particlespawners. #5492

red-001 commented Mar 31, 2017 •

edited

sfan5 Apr 1, 2017

nerzhul Apr 1, 2017

nerzhul Apr 1, 2017

red-001 Apr 1, 2017

nerzhul Apr 6, 2017

red-001 Apr 7, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

SmallJoker Apr 2, 2017

rubenwardy Apr 2, 2017

rubenwardy Apr 2, 2017

red-001 Apr 3, 2017

sofar commented Apr 6, 2017

bigfoot547 commented Apr 8, 2017

SmallJoker Apr 8, 2017 •

edited

SmallJoker Apr 8, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sfan5 Apr 29, 2017

sofar commented May 4, 2017

red-001 commented Jun 28, 2017

[CSM] Add local particles and particlespawners. #5492

[CSM] Add local particles and particlespawners. #5492

Conversation

red-001 commented Mar 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofar commented Apr 6, 2017

bigfoot547 commented Apr 8, 2017

SmallJoker Apr 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofar commented May 4, 2017

red-001 commented Jun 28, 2017

red-001 commented Mar 31, 2017 •

edited

SmallJoker Apr 8, 2017 •

edited