Fire PlayerInitialSpawnEvent (#922)

[clabe45]

A PlayerInitialSpawnEvent is now fired the first time spawnAt is called for a player.

[CLAassistant]

All committers have signed the CLA.

[SHADOWDANCH]

I think PlayerInitialSpawnEvent should be called not on first join but just before spawning player entity in world. Paper original patch here (removed)

[aramperes]

@SHADOWDANCH Contributions based on source code (patches or otherwise) from CraftBukkit or any of its forks are prohibited by our CLA. I've removed the link in your comment as a precaution.

If the documentation is insufficient for this particular event, I suggest contacting the PaperMC team on Discord for clarification and/or suggesting they update their docs.

[SHADOWDANCH]

@Momothereal if i myself show how this event works on paper (screenshots or something) it be correct?

[aramperes]

@SHADOWDANCH Yes, that is considered reverse-engineering and it is encouraged by the project :)

[SHADOWDANCH]

So i created simple test plugin and here is source code. And here results i get
Output on first spawn:

playerName: SHADOWDAN_
hasPlayedBefore: false
spawnLocation: Location{world=CraftWorld{name=world},x=-169.5,y=64.0,z=256.5,pitch=0.0,yaw=0.0}

Output after second spawn:

playerName: SHADOWDAN_
hasPlayedBefore: true
spawnLocation: Location{world=CraftWorld{name=world},x=-169.5,y=64.0,z=256.5,pitch=-3.45,yaw=5.3270264}

Ss you can see event called on second spawn too. So Initial in current scope means not player first join to world but first spawning player entity. Changing location in PlayerInitialSpawnEvent changes location where player entity be spawned and in if change location in PlayerSpawnLocationEvent player be "teleported" to new location and not initially spawned on it

[clabe45]

@SHADOWDANCH so you're saying it should be fired every time the player joins?

[SHADOWDANCH]

@clabe45 yes every time before player spawning in world

[clabe45]

@SHADOWDANCH so before spawnAt is called?

[SHADOWDANCH]

@clabe45 yes, i think

[mastercoms]

I would do it in GlowPlayer#join. Sorry for not doing my research first.

[clabe45]

@Momothereal what do you think?

[aramperes]

This is tricky because the event has a setter, setSpawnLocation. In my opinion, the result of the setter should be the initial location of the player entity, which is currently being set in the constructor:

Glowstone/src/main/java/net/glowstone/entity/GlowPlayer.java

Lines 579 to 580 in 760bff1

	public GlowPlayer(GlowSession session, GlowPlayerProfile profile, PlayerReader reader) {
	super(initLocation(session, reader), profile);

This is due to our Entity implementation which requires a Location in the constructor. I think this has been a limitation in other parts of Glowstone before which should be revamped.

So I looked for where PlayerSpawnLocationEvent was being called, and I found this in GlowEntity's constructor:

Glowstone/src/main/java/net/glowstone/entity/GlowEntity.java

Lines 319 to 325 in 760bff1

	// this is so dirty I washed my hands after writing it.
	if (this instanceof GlowPlayer) {
	// spawn location event
	location = EventFactory.getInstance()
	.callEvent(new PlayerSpawnLocationEvent((Player) this, location))
	.getSpawnLocation();
	}

It seems like the PlayerInitialSpawnEvent call should be identical to this one, potentially right before this event. Again, I would confirm the difference between these two events with the Paper team.

@SHADOWDANCH Maybe you could test the ordering of these events in your plugin?

[SHADOWDANCH]

@Momothereal yes.
Updated source code here
Output i get:

UUID of player SHADOWDAN_ is 8648480e-b14a-383f-8242-1f3a0b2f4082
PlayerInitialSpawnEvent called! playerName: SHADOWDAN_
hasPlayedBefore: true
spawnLocation: Location{world=CraftWorld{name=world},x=-155.5272637568838,y=64.0,z=250.0834669566757,pitch=9.300014,yaw=52.427734}
PlayerSpawnLocationEvent called! playerName: SHADOWDAN_
SHADOWDAN_[/127.0.0.1:3195] logged in with entity id 79 at ([world]-155.5272637568838, 64.0, 250.0834669566757)

So PlayerInitialSpawnEvent called before PlayerSpawnLocationEvent

[aramperes]

@SHADOWDANCH Cool. Could you test that setting the PlayerInitialSpawnEvent#setSpawnLocation value carries over to the PlayerSpawnLocationEvent#getSpawnLocation in the event handler?

If it does carry over, then I think the correct implementation is to copy the firing of PlayerSpawnLocationEvent right before it in the GlowEntity constructor.

[SHADOWDANCH]

@Momothereal yes.
Updated source code here
Output:

UUID of player SHADOWDAN_ is 8648480e-b14a-383f-8242-1f3a0b2f4082
PlayerInitialSpawnEvent called! playerName: SHADOWDAN_
hasPlayedBefore: true
spawnLocation: Location{world=CraftWorld{name=world},x=-156.70885792222927,y=74.0,z=255.04264081124023,pitch=12.450013,yaw=20.777733}
PlayerSpawnLocationEvent called! playerName: SHADOWDAN_
spawnLocation: Location{world=CraftWorld{name=world},x=-156.70885792222927,y=74.0,z=255.04264081124023,pitch=12.450013,yaw=20.777733}
SHADOWDAN_[/127.0.0.1:3933] logged in with entity id 106 at ([world]-156.70885792222927, 74.0, 255.04264081124023)

SpawnLocation same in this events so it passes from PlayerInitialSpawnEvent to PlayerSpawnLocationEvent

[clabe45]

Ok, points noted. Could we move that player-specific code in GlowEntity constructor to GlowPlayer? I think that would make more sense and it would make my job easier, unless if it would break something.

[aramperes]

Unfortunately, moving this code to GlowPlayer is not possible since it would be fired after the GlowEntity, GlowLivingEntity, and GlowHumanEntity constructors are done executing. The GlowEntity constructor depends on the spawnLocation to determine which world to allocate the entity to.

The revamp I mentioned would move the allocation portion out of the GlowEntity constructor, but that is not in the scope of this PR. If you want to simply implement the event with our current architecture, it has to be fired in the GlowEntity constructor just like PlayerSpawnLocationEvent.

Also, looking at the GlowEntity constructor totally shows a bug with the world being chosen before the event is fired. Maybe you could fix this by declaring the world variable after the if-statement?

Glowstone/src/main/java/net/glowstone/entity/GlowEntity.java

Lines 317 to 327 in 760bff1

	world = (GlowWorld) location.getWorld();
	server = world.getServer();
	// this is so dirty I washed my hands after writing it.
	if (this instanceof GlowPlayer) {
	// spawn location event
	location = EventFactory.getInstance()
	.callEvent(new PlayerSpawnLocationEvent((Player) this, location))
	.getSpawnLocation();
	}
	server.getEntityIdManager().allocate(this);
	world.getEntityManager().register(this);

[clabe45]

Ahh ok, so I will fire the PlayerInitialSpawnEvent before PlayerSpawnLocationEvent in the GlowEntity constructor. The location for PlayerSpawnLocationEvent and the player's location itself will depend on the result from PlayerInitialSpawnEvent. However, what's the good of setting the local variable location in the constructor of GlowEntity after it is cloned in the beginning of the constructor? I don't know how to work with that.

[aramperes]

Hm that's interesting. The field is set at the beginning so that calling event.getPlayer().getLocation() still works, but it seems like the output of the event is never set. Something like:

public GlowEntity(Location location) {
    // Set initial locations for event calls
    this.origin = location.clone();
    this.previousLocation = location.clone();
    this.location = location.clone();

    // this is so dirty I washed my hands after writing it.
    if (this instanceof GlowPlayer) {
        // TODO: Fire PlayerInitialSpawnEvent

        // spawn location event
        location = EventFactory.getInstance()
            .callEvent(new PlayerSpawnLocationEvent((Player) this, location.clone()))
            .getSpawnLocation().clone();

        // Update locations again
        this.origin = location.clone();
        this.previousLocation = location.clone();
        this.location = location.clone();
    }

    // ID allocation
    world = (GlowWorld) location.getWorld();
    world.getServer().getEntityIdManager().allocate(this);
    world.getEntityManager().register(this);
}

[clabe45]

Ok noted.

[clabe45]

I tried that, but origin, previousLocation and location are all final. Idk how to set a location to another location without setting it.

[aramperes]

You can use Position.copyLocation(location, this.location) etc. to copy the contents of the first location to the second one.

[aramperes]

LGTM!

[clabe45]

Awesome! This will be the first PR of many :D

[mastercoms]

Thank you for your contribution to Glowstone!

[clabe45]

My pleasure