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

Minecraft Low Level API: First draft #44

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

Minecraft Low Level API: First draft #44

wants to merge 8 commits into from

Conversation

dbrgn
Copy link
Member

@dbrgn dbrgn commented Oct 3, 2014

Still WIP, I want to do a few tests before this is merged.

In the meantime, feel free to look over the code, I'd be happy about feedback.

I'm not sure how we should deal with the connection class. It's probably better if we keep it separate from the mcpi version, then we can improve it without having to deal with low level backwards compatibility (e.g. people trying to catch specific exceptions).

Once this is merged, it should fix #10.

@hashbangstudio
Copy link
Contributor

For the settings() functions would suggest kwargs in appropriate namespace.
For example:

world
To make blocks in world unbreakable (key could possibly be made more readable as world_unbreakable, but immutable is the text that Minecraft expects in command)
mc.world.setting(world_immutable=True)
To display nametags above player (can be seen in follow or fixed camera mode) assume can be seen above other players from first person view but only had one player in minecraft pi edition so far
mc.world.setting(nametags_visible=True)

player
Not sure if this is a global setting for all players, in which case is better set in world namespace or if can be done per player, as only had one player in minecraft pi edition so far. Not referenced in the Specs doc supplied with mojang api but exists in the api and works.

To make player auto jump up small block height differences when moving around
mc.player.setting(autojump='on')
or
mc.player.setting(autojump=True)

"""
The command base class.
"""
_func_prefix = ''
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to find a nicer name for this, it took me a few minutes to track down what it's used for. However I'm not sure what a nicer name would be…

Copy link
Member Author

Choose a reason for hiding this comment

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

If you know a better one, let me know :)

@ghickman
Copy link
Member

ghickman commented Oct 3, 2014

@dbrgn – This is a great start!

@jonathanfine jonathanfine mentioned this pull request Oct 4, 2014
@doismellburning
Copy link
Member

Re minecraft.core.Connection - since:

  1. you're mostly duplicating mcpi.Connection
  2. whatever people's opinions on "the API as a whole" I think we can all agree that it would be nice to improve mcpi.Connection

would you mind using that and patching/overriding as appropriate please?

@dbrgn
Copy link
Member Author

dbrgn commented Oct 19, 2014

@hashbangstudio: I did a first attempt at implementing the setting functions (85e21fd). What do you think?

I'm also not sure if autojump is a global setting or not. I've also not yet played around with multiple players, but it's something we'd need to figure out before releasing this code.

@dbrgn
Copy link
Member Author

dbrgn commented Oct 19, 2014

@doismellburning: Didn't we decide that we should leave the legacy API as-is to be able to improve the new low level implementation without any backwards incompatibility fears? It limits the flexibility regarding the implementation, and implementation changes could affect the legacy API.

I'd certainly be up for improving the connection class, the current one causes a lot of random issues and has nonexistant error reporting. But if we share a codebase for the legacy low level API and the new low level API we limit ourselves.

@dbrgn
Copy link
Member Author

dbrgn commented Oct 19, 2014

Another note: Last week I did two 4.5h courses for 4 kids each (around 12-14y) where we introduced them to Minecraft and mcpi. Some of the takeaways regarding the API:

  • The difference between SetPos and SetTilePos is confusing. Is SetPos and GetPos actually needed anywhere? I can't come up with an example where floats are required instead of integers.
  • Flat arguments are probably better than a position object, although a position object could certainly give some benefits. The current mcpi implementation does this by enabling the unpacking of a Vec3 instance into three variables (x, y, z = Vec3(1, 2, 3)).
  • Relative coordinates would certainly be useful, as we basically did things like x, y, z = mc.getPlayerPos(); mc.setBlocks(x+1, y+1, z+1, x+10, y+15, z+5) all day long. It should probably belong in the high level API though.

@coveralls
Copy link

Coverage Status

Coverage increased (+34.09%) when pulling 4e17ddf on lowlevel_api into e697c9c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+34.09%) when pulling 4e17ddf on lowlevel_api into e697c9c on master.

@doismellburning
Copy link
Member

@doismellburning: Didn't we decide that we should leave the legacy API as-is

Leaving the legacy API as-is, yes - I'm very much pro improving the implementation underneath though, partly because it's a faster route to improvement.

to be able to improve the new low level implementation without any backwards incompatibility fears? It limits the flexibility regarding the implementation, and implementation changes could affect the legacy API.

I'd like to see specific cases here

I'd certainly be up for improving the connection class, the current one causes a lot of random issues and has nonexistant error reporting

Agreed

But if we share a codebase for the legacy low level API and the new low level API we limit ourselves.

I'm not talking about sharing everything, but right now, your Connection is almost a straight copy-paste, which I very much object to

Last week I did two 4.5h courses for 4 kids each (around 12-14y)

Nicely done :) Interesting data!

@dbrgn
Copy link
Member Author

dbrgn commented Oct 19, 2014

OK, I can move the improvements into the original connection module. But I'd prefer to do it in a separate PR later on. Is that fine?

@doismellburning
Copy link
Member

<3 small focussed PRs

@dbrgn
Copy link
Member Author

dbrgn commented Oct 19, 2014

So how should we handle this PR?

@doismellburning
Copy link
Member

Retarget it onto your Connection PR?

@dbrgn
Copy link
Member Author

dbrgn commented Oct 20, 2014

PRs can't be retargeted :) Feel free to +9000 this issue. isaacs/github#18

@doismellburning
Copy link
Member

Sure, sorry, I keep using that term as shorthand

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.

Low level APIs
5 participants