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

PlaceBlock #511

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

PlaceBlock #511

wants to merge 6 commits into from

Conversation

XBagon
Copy link
Contributor

@XBagon XBagon commented Sep 1, 2023

Objective

Fixes #503

Solution

Adds PlaceBlockCommand (used with Command::add), which places a block according to infos about itself, the entity placing the block and its environment. Tries to mimic client behaviour as close as possible.

This is the foundation and definitely hasn't all rules implemented. It's a proposal for the form and helper structs/functions to make implementing the rest of the rules simpler and more elegant. I tried to document my sources, open problems and other thoughts in the comments.

Maybe it makes sense to merge this is in a unfinished state, when most common interactions are handled and it generelly is preferable over set_block. At the moment it uses a lot of unimplemented! to catch missing features, those could be converted into silent ones.

impl PlacementRule {
//TODO: WIP
pub const fn from_kind(kind: BlockKind) -> Self {
match kind {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this approach isn't really maintainable. I don't want to manually update this list every time a new Minecraft version is released.

I think we should investigate how Vanilla is mapping the blocks to their placement rules and extract that information if possible. If not, then perhaps we could use some heuristic based on properties, but this is not my desired solution.

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 don't think there's much room for alternatives, if we want to mimic the client behaviour. Minecraft seems to hardcode these rules for these sets of blocks as well. As you said these rules are very complicated and I don't see a good way around this. Having a good default, recognizing similarities and thus being able to deduplicate code are things I try to do and hopefully can limit the maintanence cost. Also adding new blocks to these rules shouldn't be too much work.

Only alternative I can think of is dropping this in the scope of Valence entirely and make a crate for vanilla behaviour, which is allowed to be behind Valence and the newest Minecraft update a bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah minecraft implements placement behavior for each block by implementing this function:

public BlockState getPlacementState(ItemPlacementContext ctx)

For example, this is the impl in EnderChestBlock:

public BlockState getPlacementState(ItemPlacementContext ctx) {
        FluidState fluidState = ctx.getWorld().getFluidState(ctx.getBlockPos());
        return (BlockState)((BlockState)this.getDefaultState().with(FACING, ctx.getHorizontalPlayerFacing().getOpposite())).with(WATERLOGGED, fluidState.getFluid() == Fluids.WATER);
    }

It might be feasible to have some codegen that reads the with() calls and derives the correct placement rule for the block. eg, if with(FACING, ...) is present, the block probably should face the player.

Copy link
Contributor

@Bafbi Bafbi Sep 1, 2023

Choose a reason for hiding this comment

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

so extracting properties ? cause we already extract them, but yea I don't see placement rules that couldn't be derive from some properties.

I don't know if it would be a good idea, but we could bruteforce blockstate for every context them infer the rules from that

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.

Add block placement rules.
4 participants