FountainMC / FountainAPI

A 'simple but beautiful' Minecraft Server Plugin API
https://fountainmc.org/
MIT License
5 stars 5 forks source link

Use BlockState instead of Blocks #2

Closed Techcable closed 8 years ago

Techcable commented 8 years ago

Blocks are a little extreme object oriented-wise. CraftBlock is a inefficient utility object and has no corresponding object in the minecraft code. Using BlockStates (like minecraft does), would make a lot more sense, would be more efficient.

I think it'd also be better for plugins since BlockStates are immutable and represent the actual data behind a block, not the block itself. Since they'd be immutable they could also have an inheritance hierarchy and we can represent type-specific information with sub-interfaces. (Bukkit has this too but in a separate API added later as an afterthought)

phase commented 8 years ago

Seems like a nice plan.

phase commented 8 years ago

Closed in https://github.com/FountainMC/FountainAPI/commit/6b9a3b785dbcea66bdc3dda06c8453d6a4f26099

phase commented 8 years ago

Woah, no this is not closed. No clue what I was doing.

This whole "3am programming" thing isn't working out so great.

phase commented 8 years ago

What should these BlockStates contain?

28andrew commented 8 years ago

Maybe things similar to https://hub.spigotmc.org/javadocs/spigot/org/bukkit/block/BlockState.html

phase commented 8 years ago

Stash Link

That's what I had in mind, except their would only be on BlockState per coords.

28andrew commented 8 years ago

Yeah, a "Block" in Spigot is represented by coords.

Techcable commented 8 years ago

Except that it shouldn't have coordinates, it should just represent a block's data. I'm working on implementing all the block data (not tile entities) in minecraft right now.

PizzaCrust commented 8 years ago

So, BlockPosition represents a block's position inside of the World. And, the BlockState contains all of the data of the block?

Techcable commented 8 years ago

@PizzaCrust Yes, and since blocks can only have 16 values at max, there will only be 16 instances of a BlockState at max.

PizzaCrust commented 8 years ago

If the BlockStates are immutable, will it be so:

Techcable commented 8 years ago

Kinda like this:

BlockPosition position = new BlockPosition(world, x, y, z);
BlockState state = world.getBlock(position);
if (state instanceof PoweredBlock) {
    state = ((PoweredBlock) state).withPower(PoweredBlock.MAXIMUM_POWER); // Notice how this is a new object, and the existing state is the same
}
world.setBlock(position, state);
phase commented 8 years ago

How would I get an instance of a BlockState from an id and meta? I assume it's not

BlockState state = new BlockState(1, 3);

Will it be another World method?

BlockState grass = world.getBlockState(2);
PizzaCrust commented 8 years ago

@Techcable

Nice!

Techcable commented 8 years ago

@phase It'd be a server method, since it doesn't need a world.

PizzaCrust commented 8 years ago

@Techcable your maven repository doesn't work on gradle.

Techcable commented 8 years ago

Fixed by a807df3913c69efedfd4644be7affba468823f81

PizzaCrust commented 8 years ago

How would one implement getMaterial(String name), all I can think of is circularity issues.

Techcable commented 8 years ago

@PizzaCrust Good point, we might want to move them to 'VanillaMaterials'.

PizzaCrust commented 8 years ago

I think this Material class should just be a enum.

Techcable commented 8 years ago

@PizzaCrust We want to be able to eventually support mods 😉

PizzaCrust commented 8 years ago

Mods can add enums via reflection.

Techcable commented 8 years ago

@PizzaCrust We want to prevent this We shouldn't use an enumeration when there is a possibility for additional items.

PizzaCrust commented 8 years ago

If the material class is a enum, they could do this.

phase commented 8 years ago

We don't want mods to bend to our needs, we want plugins to. That's the problem MCPC had.