search

GlowstoneMC / Glowstone-Legacy

An open-source server for the Bukkit Minecraft modding interface
Other
363 stars 122 forks source link

Improved Block/ItemType system #566

Closed Tonodus closed 9 years ago

Tonodus commented 9 years ago

This PR is based on SpaceManiacs suggestions, but I think it's highly recommended to discuss the implemented component system, before any testing is done.

The diff is quite huge, but a great amount is only single word changes or moved code - f.e. DefaultBlockType#rightClickBlock comes from BlockType#rightClickBlock.

(fixes #536)

Glowkit: https://github.com/GlowstoneMC/Glowkit/pull/60

turt2live commented 9 years ago

!!! @Tonodus Doesn't compile :(

(see travis)

Tonodus commented 9 years ago

@SpaceManiac fixed @turt2live yeah... this needs https://github.com/GlowstoneMC/Glowkit/pull/60

GLOWKIT: 60

turt2bot commented 9 years ago

@Tonodus This PR has been associated with GlowstoneMC/Glowkit#60 - Additions for improved BlockType-system (adds hay, ender-portal-frame, improved lever-facing)

turt2live commented 9 years ago

I'm putting this on hold until various other block-related PRs are pulled, such as #249 (physics) and #284 (explosions).

turt2live commented 9 years ago

@Tonodus This pull request is no longer mergeable. Please resolve the issue as soon as possible, thanks!

Tonodus commented 9 years ago

@turt2live ps: the "on hold" isn't needed anymore. @SpaceManiac any chance this get pulled once or can this be closed due to the blocks branch?

SpaceManiac commented 9 years ago

@Tonodus I'd be interested in your feedback on the current state of things on the blocks branch, and how well you think the system shown there solves things.

turt2live commented 9 years ago

@Tonodus Your pull request has been unmergeable for 3 days now. Please resolve the conflicts as soon as possible.

dequis commented 9 years ago

I noticed the conflict, tried to merge without even knowing what this was about, and saw this

    public ItemFlintAndSteel() {
<<<<<<< HEAD
        super(
                new ItemTool() {
                    @Override
                    public boolean onToolRightClick(GlowPlayer player, ItemStack holding, GlowBlock target, BlockFace face, Vector clickedLoc) {

My reaction was "WTF" until i realized that this PR is another approach to what the blocks branch is doing.

I think mergeability is irrelevant since this is more of a proposal of how to design stuff.

Tonodus and SpaceManiac should talk about this stuff if they haven't done so yet. Personally I like what spacemaniac did with the blocks branch, but I also like the yaml stuff that tonodus added in #541, for example

turt2live commented 9 years ago

Held: Pending analysis of block system within Glowstone.

Tonodus commented 9 years ago

@SpaceManiac @turt2live It's been a while since I've responded here, but I didn't forget about this: After I did much more investigation on SpaceManiac's work, I think this PR can be closed now.

His draft includes much more functionality than this PR does and seems to be more future-proof to me, especially when we're looking at Minecraft's new BlockState system or support of sponge one day, while I doesn't miss functionality added by this PR.

But the most important: If we only compare this PR and his BlockBehavior, our solutions are quite similar (cf. ListBlockBehavior and DefaultBlockType) - biggest difference is the way, to say that certain functionality is not provided. Throwing a Signal instead of returning null has some advantages and I don't see any disadvantages for now.

Finally, I tried some experiments suitable for my solution with SpaceManiacs branch. I didn't notice any missing functionality or other disadvantages due to his behavior-approach.

I'm sorry, I'm not able to provide improvements for the blocks-branch right now. If anyone thinks this PR should be re-opened, because my implementation has advantages I don't see right now, feel free to respond.