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 21 commits into
base: master
Choose a base branch
from
Open

implement beds , respawn anchors #938

wants to merge 21 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 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.

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
server/session/handler_player_action.go Show resolved Hide resolved
server/world/sleep.go Outdated Show resolved Hide resolved
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

server/item/item.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/block/respawn_anchor.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/session/handler_player_action.go 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
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
server/player/player.go Outdated Show resolved Hide resolved
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

@FDUTCH
Copy link
Contributor Author

FDUTCH commented Dec 1, 2024

Please fix merge conflicts

bro , I'm dead ☠️
I don't know how to resolve merge conflict

# Conflicts:
#	server/player/handler.go
#	server/player/player.go
#	server/session/session.go
#	server/world/tick.go
#	server/world/world.go
@DaPigGuy DaPigGuy mentioned this pull request Dec 15, 2024
89 tasks
@DaPigGuy DaPigGuy requested review from Sandertv and removed request for DaPigGuy December 16, 2024 07:11
Copy link
Contributor

@xNatsuri xNatsuri left a comment

Choose a reason for hiding this comment

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

most of the pr looks good just from a quick glance saw this issue

server/block/bed.go Outdated Show resolved Hide resolved
Copy link
Member

@Sandertv Sandertv left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've left some more comments.

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/player/handler.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/session/session.go Outdated Show resolved Hide resolved
server/world/sleep.go Show resolved Hide resolved
@FDUTCH FDUTCH requested review from Sandertv and DaPigGuy December 18, 2024 15:42
@DaPigGuy
Copy link
Member

Please fix merge conflicts and update to use the translation changes from #961 for the bed messages

# Conflicts:
#	server/player/player.go
#	server/session/session.go
@FDUTCH FDUTCH requested a review from DaPigGuy December 23, 2024 15:38
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.

5 participants