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

Attached Particle Spawners #4409

Merged
merged 1 commit into from Oct 13, 2016

Conversation

@raymoo
Copy link
Contributor

commented Aug 4, 2016

Implements #567 (attached particle spawners).

Here is the code for a test mod implementing rain: http://lpaste.net/174254

The particles in the demo are shown to all players, so you can also use it to test if the spawners get deleted when an object is removed, by entering with multiple clients and logging out of one.

@juhdanad

View changes

src/particles.cpp Outdated
if (attached != NULL)
{
// CAO coordinates in irrlicht units, but particles are
// not.

This comment has been minimized.

Copy link
@juhdanad

juhdanad Aug 4, 2016

Contributor

You could use BS there.

@juhdanad

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2016

If ActiveObjects contained references to the particle spawners, the spawner would be deleted when the object is deleted/unloaded. The class ActiveObject could extend ParticleSpawner to spawn particles. Or ParticleManager could also support ActiveObjects as spawners. Or you could create ParticleSpawner::removeSpawner() which the ActiveObject calls when it is unloaded or deleted.
I hope I could help.

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Aug 4, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

Thanks @juhdanad. I've already started implementing that part, and have gone the route of having the server manage the lifetime of attached particle spawners.

@raymoo raymoo changed the title (WIP) Attached Particle Spawners Attached Particle Spawners Aug 4, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

Server-side lifetime handling implemented.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

I forgot to document this.

EDIT: documented

@Bremaweb

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2016

I love it, seems to work really well.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

I noticed that you can use 0 as a "null object id" so I changed the code to use u16 instead of s32 for sending and handling ids.

@raymoo raymoo force-pushed the raymoo:attached_spawners branch 2 times, most recently Aug 5, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

Rebased and squashed

@raymoo raymoo force-pushed the raymoo:attached_spawners branch to 79b7a3e Aug 5, 2016

@sofar

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

Question: Can we get an interface that is similar to player:set_attach() and player:detach() ? Maybe that will make it easier to avoid the "0" id problem? Just a thought.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2016

@sofar It would only avoid it in the add_particlespawner packet, which I don't think is a big deal. It could already be avoided in the client by keeping a table of attachments, as it is done on the server, but I didn't do that because handling and tracking the offset of particles logically belongs in the particle spawner code.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2016

Also using a set_attach would either require an API change to add_particlespawner, which currently returns a number, or adding a set_attach to the metatable for numbers, which doesn't work if we find another "id-represented thing" that we want to add a set_attach to.

@@ -156,6 +156,13 @@ LuaEntitySAO::~LuaEntitySAO()
if(m_registered){
m_env->getScriptIface()->luaentity_Remove(m_id);
}

std::set<u32>::iterator it;
std::set<u32>::iterator begin = m_attached_particle_spawners.begin();

This comment has been minimized.

Copy link
@sofar

sofar Sep 16, 2016

Member

I think in most of the code you'll find that begin/end are just omitted and m_attached_particle_spawners.begin() is just used directly in the for( loop.

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 17, 2016

Author Contributor

I'll put them inline but I'm not going to make a line with 100+ columns

u32 id = addParticleSpawner(exptime);
m_particle_spawner_attachments[id] = attached_id;
ServerActiveObject *obj = getActiveObject(attached_id);
if (obj != NULL) {

This comment has been minimized.

Copy link
@sofar

sofar Sep 16, 2016

Member

I prefer if (obj) {

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 17, 2016

Author Contributor

You just said to use iterator methods inline since the rest of the code does it, but in this file the code typically uses != NULL.

v3f attached_offset = v3f(0,0,0);
if (m_attached_id != 0) {
ClientActiveObject *attached =
env->getActiveObject(m_attached_id);

This comment has been minimized.

Copy link
@sofar

sofar Sep 16, 2016

Member

fits on one line?

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 17, 2016

Author Contributor

it goes a bit above 80 columns, but I guess that the style guide says that's ok

if (m_attached_id != 0) {
ClientActiveObject *attached =
env->getActiveObject(m_attached_id);
if (attached != NULL) {

This comment has been minimized.

Copy link
@sofar

sofar Sep 16, 2016

Member

if (attached) {

@@ -3200,11 +3202,19 @@ u32 Server::addParticleSpawner(u16 amount, float spawntime,
peer_id = player->peer_id;
}

u32 id = m_env->addParticleSpawner(spawntime);
u16 attached_id = attached == NULL ? 0 : attached->getId();

This comment has been minimized.

Copy link
@sofar

sofar Sep 16, 2016

Member

u16 attached_id = attached ? attached->getId() : 0; ?

@sofar
Copy link
Member

left a comment

Looks pretty good.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

I've made your changes, just making sure it builds

@raymoo raymoo force-pushed the raymoo:attached_spawners branch from 79b7a3e Sep 17, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

actually hold on, I missed one.

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Sep 17, 2016

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Oct 6, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

Rebased.

src/particles.cpp Outdated
attached_offset = attached->getPosition() / BS;
} else {
unloaded = true;
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Oct 6, 2016

Member

removal of braces possible:

if (attached)
    attached_offset = attached->getPosition() / BS;
 else
    unloaded = true;
src/server.cpp Outdated
id = m_env->addParticleSpawner(spawntime);
} else {
id = m_env->addParticleSpawner(spawntime, attached_id);
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Oct 6, 2016

Member

removal of braces possible

@sfan5 sfan5 added the One approval label Oct 6, 2016

@sfan5

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

I have to say:

Really really good job! 👍

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Oct 6, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

@sfan5 Thanks, also made the suggested changes.

@sofar

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

👍 from me, very happy to see this patch :)

src/environment.cpp Outdated
{
u32 id = addParticleSpawner(exptime);
m_particle_spawner_attachments[id] = attached_id;
ServerActiveObject *obj = getActiveObject(attached_id);

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

merge this in the condition

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

But I use obj a couple lines later, won't I have to repeat getActiveObject then?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

no you use it only in the condition before, then you can do this affectation in the condition

src/server.cpp Outdated
@@ -3163,6 +3164,7 @@ void Server::spawnParticle(const std::string &playername, v3f pos,
if (playername != "") {
Player* player = m_env->getPlayer(playername.c_str());
if (!player)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

you should remove this

src/serverobject.cpp Outdated
@@ -99,3 +99,12 @@ bool ServerActiveObject::setWieldedItem(const ItemStack &item)
return false;
}

void ServerActiveObject::attachParticleSpawner(u32 id)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

please inline this function

src/serverobject.cpp Outdated
m_attached_particle_spawners.insert(id);
}

void ServerActiveObject::detachParticleSpawner(u32 id)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

same here, inline by moving definition to header

src/particles.cpp Outdated
v3f attached_offset = v3f(0,0,0);
if (m_attached_id != 0) {
ClientActiveObject *attached = env->getActiveObject(m_attached_id);
if (attached) {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

merge condition & previous line

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

I use attached in the next line, won't I have to call env->getActiveObject again?

src/environment.cpp Outdated
if (sao != NULL && remove_from_object) {
sao->detachParticleSpawner(id);
}
} catch (...) {}

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

why there is a try catch here ?

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

The "at" method throws an exception if something isn't there. Often there won't be, since particle spawners might not be attached

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

please catch the corresponding exception then not all

src/environment.cpp Outdated
}

void ServerEnvironment::deleteParticleSpawner(u32 id,
bool remove_from_object)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

you can move this on the upper line

@nerzhul nerzhul added this to the 0.4.15 milestone Oct 12, 2016

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Oct 12, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

@nerzhul Made some of your requested changes, responded or asked for clarification on others.

src/particles.cpp Outdated
v3f attached_offset = v3f(0,0,0);
if (m_attached_id != 0) {
ClientActiveObject *attached = env->getActiveObject(m_attached_id);
if (attached)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

please do the decl/affectation in the condition here too

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

I use attached on the next line, am I supposed to duplicate the method calls?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

no because it's used in the condition, in fact with this method you do the test for the decl/assignment directly without creating a variable out of the needed scope

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

I didn't know you could put declarations in the condition, thanks.

@raymoo raymoo force-pushed the raymoo:attached_spawners branch Oct 12, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

@nerzhul Ok, did the things.

@raymoo raymoo force-pushed the raymoo:attached_spawners branch 2 times, most recently Oct 12, 2016

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

Switched from using at to find, since at and its associated exception are C++11 features.

src/environment.cpp Outdated
sao->detachParticleSpawner(id);
}
}
m_particle_spawner_attachments.erase(id);

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 12, 2016

Member

i think this call should be in the condition, no?

This comment has been minimized.

Copy link
@sfan5

sfan5 Oct 12, 2016

Member

Yes it should, however .erase() doesn't do anything when the key is not found in the array, so it doesn't matter in practice.

This comment has been minimized.

Copy link
@raymoo

raymoo Oct 12, 2016

Author Contributor

Right, though it didn't do any harm being outside the condition. (Pushed a new version with the erase inside the condition)

@raymoo raymoo force-pushed the raymoo:attached_spawners branch to ce3a428 Oct 12, 2016

@nerzhul

This comment has been minimized.

Copy link
Member

commented Oct 13, 2016

Okay for me

@nerzhul nerzhul merged commit c9e7a27 into minetest:master Oct 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@raymoo raymoo deleted the raymoo:attached_spawners branch Oct 13, 2016

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.