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

Adds missing spawn events for items #74

Closed
wants to merge 1 commit into from
Closed

Adds missing spawn events for items #74

wants to merge 1 commit into from

Conversation

Ryxias
Copy link

@Ryxias Ryxias commented Apr 12, 2019

To be consistent with the @event Item#spawn event that is fired in Room#spawnItem

Addresses #73

@seanohue
Copy link
Contributor

Code-wise it looks good to me, design-wise I have a question/scenario:

I don't think this would break anyone's code and it definitely makes sense to emit an event in those spots. But I wonder if someone would still be able to differentiate whether the item was spawning for the first time or being rehydrated or something? If not, I could see that causing bugs. Maybe the container it is spawned into could be passed as an argument? (the room, npc, item container, etc.)

@Ryxias
Copy link
Author

Ryxias commented Apr 12, 2019

That is a very good point @seanohue; this pattern could cause multiple spawn events to be emitted on any single creation attempt.

A workaround would be to detect if an item has been spawned already before emitting the event... but that seems kind of, inelegant. I'll think about it

@seanohue
Copy link
Contributor

Maybe something like item.emit('spawn', room) or item.emit('spawn', item); so that one could check against where it is spawned. That said, such a change could always be made later in case there is something that breaks or is otherwise backwards-incompatible.

@seanohue
Copy link
Contributor

Has this been tested in the wild yet?

@shawncplus shawncplus added the feedback PR/Issue has pending feedback label Jun 3, 2019
@shawncplus shawncplus added this to the 3.2 milestone Aug 22, 2019
@azigler
Copy link

azigler commented Jun 6, 2020

I've tested this and I'm not encountering duplicate spawn events per item. All of these new events are emitted only during hydration, which only happens when:

  • the server boots and initially spawns the item,
  • an area updates and triggers a respawn of the item, or
  • a player holding or equipping the item logs on

An item won't be spawned/hydrated in multiple places and the event won't be emitted multiple times per item, as each spawn/hydrate creates a unique item.

Separately, there's no way to differentiate between an item that's spawned for the first time and an item that was simply rehydrated. If a player possesses an item (either in their inventory or equipped) that later respawns back in its original area (thus creating a second copy of the item), a spawn event will fire when the player logs in and hydrates their inventory or equipment while they possess the original item, and a spawn event will fire at the time when the replacement item respawns at its original location. Those are two separate items, however, so each should trigger their own Item#spawn event.

These changes look good to me!

@seanohue
Copy link
Contributor

Came across this again while going through open, public PRs that I'm mentioned in.
This looks ready to merge but unfortunately I don't have merge permissions at this point.

Hoping to draw attention to it with a comment.

@Ryxias Ryxias closed this by deleting the head repository Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback PR/Issue has pending feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants