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

implement beds , respawn anchors #938

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

implement beds , respawn anchors #938

wants to merge 11 commits into from

Conversation

FDUTCH
Copy link
Contributor

@FDUTCH FDUTCH commented Nov 17, 2024

No description provided.

Copy link
Member

@TwistedAsylumMC TwistedAsylumMC left a comment

Choose a reason for hiding this comment

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

Noticed a few problems after a quick look

server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/world/sound/block.go Show resolved Hide resolved
Comment on lines 57 to 58
t.w.set.RequiredSleepTicks--
if t.w.set.RequiredSleepTicks-1 <= 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic correct? You're decrementing the ticks and then checking if the tick below is <= 0?

server/world/world.go Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
Copy link
Member

@TwistedAsylumMC TwistedAsylumMC left a comment

Choose a reason for hiding this comment

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

Still a few of my previous comments that have not been addressed, found some more issues after another look through

Comment on lines 238 to 242
// SpawnBlock represents a block using which player can set his spawn point.
type SpawnBlock interface {
SpawnBlock() bool
Update(pos cube.Pos, u item.User, w *world.World)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use proper grammer in the docs and also document the methods of the interface. Update is a very ambiguous name and doesn't hint towards being related to spawn points

Copy link
Member

Choose a reason for hiding this comment

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

You have changed the method name but the documentation is still needing to be updated

server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
@@ -23,14 +23,15 @@ func handlePlayerAction(action int32, face int32, pos protocol.BlockPos, entityR
return errSelfRuntimeID
}
switch action {
case protocol.PlayerActionRespawn, protocol.PlayerActionDimensionChangeDone:
// Don't do anything for these actions.
Copy link
Member

Choose a reason for hiding this comment

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

If you run go fmt over this file, does it bring back this spacing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

Could you please commit that change?

Comment on lines 12 to 16
UUID() uuid.UUID
Name() string

Message(a ...any)
Messaget(key string, a ...string)
Copy link
Member

Choose a reason for hiding this comment

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

Is it absolutely necessary for a sleeper to implement these methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually only some of them are not needed

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove any that are not needed?

server/world/viewer.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
Copy link
Member

@TwistedAsylumMC TwistedAsylumMC left a comment

Choose a reason for hiding this comment

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

I have had another look, there are still unresolved comments that I have previously provided. I am not going to review this PR anymore until every single comment has been acted on or you've replied with a valid counter argument. I have gone through and resolved all the comments you have addressed but there are still many that remain

UUID() uuid.UUID

Messaget(key string, a ...string)
Sneaking() bool
Copy link
Member

Choose a reason for hiding this comment

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

This is still not resolved

// We have an actual client connected to this player: We change its position server side so that in
// the future, the client won't respawn on the death location when disconnecting. The client should
// not see the movement itself yet, though.
newPos := w.Spawn().Vec3()
p.pos.Store(&newPos)
pos, w, blockHasBeenBroken, _ := p.realSpawnPos()
Copy link
Member

Choose a reason for hiding this comment

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

I think "spawnObstructed" is a better name for this

Comment on lines 84 to 91
// allRespawnAnchorsItems returns all possible respawn anchors as items.
func allRespawnAnchorsItems() []world.Item {
all := make([]world.Item, 0, 5)
for i := 0; i < 5; i++ {
all = append(all, RespawnAnchor{Charge: i})
}
return all
}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this like I previously asked, just put this for loop in register.go for items specifically

Comment on lines 992 to 1006
// realSpawnPos returns position and world where player should be spawned.
func (p *Player) realSpawnPos() (pos cube.Pos, w *world.World, spawnBlockBroken bool, previousDimension world.Dimension) {
w = p.World()

previousDimension = w.Dimension()
pos = w.PlayerSpawn(p.UUID())
if b, ok := w.Block(pos).(block.RespawnBlock); ok && b.CanSpawn() {
return pos, w, false, previousDimension
}

// We can use the principle here that returning through a portal of a specific dimension inside that dimension will
// always bring us back to the overworld.
w = w.PortalDestination(w.Dimension())
return w.Spawn(), w, true, previousDimension
}
Copy link
Member

Choose a reason for hiding this comment

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

The name and documentation for this method is slightly misleading since nowhere does it state it actually calls w.Spawn()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll call this function ¨spawnLocation¨

@@ -23,14 +23,15 @@ func handlePlayerAction(action int32, face int32, pos protocol.BlockPos, entityR
return errSelfRuntimeID
}
switch action {
case protocol.PlayerActionRespawn, protocol.PlayerActionDimensionChangeDone:
// Don't do anything for these actions.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please commit that change?

server/block/bed.go Outdated Show resolved Hide resolved
server/block/bed.go Outdated Show resolved Hide resolved
server/block/bed.go Outdated Show resolved Hide resolved
server/block/bed.go Outdated Show resolved Hide resolved
server/block/bed.go Outdated Show resolved Hide resolved
for _, d := range cube.Directions() {
beds = append(beds, Bed{Facing: d})
beds = append(beds, Bed{Facing: d, Head: true})
}
Copy link
Member

Choose a reason for hiding this comment

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

Not having the occupied block states registered might be problematic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is that colored versions were not taken into account?

Copy link
Member

Choose a reason for hiding this comment

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

Bed color is set through the tile, not a block state. Please undo your change.

If all beds are white in the creative inventory, it is probably the same issue with the creative item entries missing NBT that banners have. See a1c98da

server/block/respawn_anchor.go Outdated Show resolved Hide resolved
vec := pos.Vec3()

if blockHasBeenBroken {
p.Handler().HandleRespawn(&vec, &w)
Copy link
Member

Choose a reason for hiding this comment

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

This is a misuse of the respawn event. The player is not actually respawning at this point. If you would like to be able to change the respawn world, make a new event, i.e. respawn change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

¨make a new event, i.e. respawn change¨ this can bee done using custom world.Provider

Copy link
Member

Choose a reason for hiding this comment

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

The respawn event should not be emitted here. The player is not actually respawning here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

soo, should I remove the event call from this function?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

server/player/player.go Outdated Show resolved Hide resolved
server/world/world.go Outdated Show resolved Hide resolved
@DaPigGuy
Copy link
Member

Please fix merge conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants