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

Low level APIs #10

Open
jonathanfine opened this issue Sep 28, 2014 · 28 comments · May be fixed by #44
Open

Low level APIs #10

jonathanfine opened this issue Sep 28, 2014 · 28 comments · May be fixed by #44

Comments

@jonathanfine
Copy link
Contributor

I think we need two low-level APIs.

  1. The legacy API, which is the current API ported to Python 3.
  2. A properly documented and tested replacement to the legacy API.

The only changes I'd like to make with the legacy API are those that support compatibility with legacy code. I think 2to3 has already made all those changes.

I'd like the replacement low-level API to be

  • Conceptually simple.
  • Small.
  • Stable
  • Easy to test.

I've done some experiments, and it seems that sending bad API requests to Minecraft can cause to freeze. We don't want that to happen, and we want to be able to reproduce this fault when it does.

So I'd also like the replacement API to provide

  • Safety - does not crash or freeze Minecraft
  • Logging - to diagnose problems when then do occur

Logging will also help with testing and debugging the high-level API.

@dbrgn
Copy link
Member

dbrgn commented Sep 28, 2014

I fully agree with this proposal :) The compat API should just be that - compatibility. All the efforts should go into a nice, polished API, see #5.

@ghickman
Copy link
Member

Definitely agree here.

Having been through the existing codebase I think we can probably take its concepts and rebuild from there, providing a porcelain wrapper over the top (most likely with clever use of an __init__ and imports) for backwards compatibility.

I think our main effort for the low level API needs to start with the socket handling. Right now it basically errors our when you get a problem, including if the socket is closed for some reason. I want this to be hidden away from the high level API and do as much error handling, reconnecting, etc to hide the use of sockets from the end user.

@bennuttall
Copy link
Member

Great. I think a compatible API that works in Python 3 is important, and an extended version that's more Pythonic and more feature rich would be ace.

Low level & high level sounds great too.

I think abstraction is good but don't want to rely on it. Functions like build_house should be basic, not full solutions. The important thing is to encourage learning and creativity, not to make a labour saving device for gamers. For example:

def build_house(x, y, z, width, height, block):
    """
    Builds a house at location (x, y, z) size width x height made of block type block
    """
    ...

This might just make a hollow cube with walls and a roof, with a door at the front. The code would be simple enough that the user could understand it and make their own function that did something else.

Thought occurs: is it more pythonic to pass in the location as a 3-tuple? Or Vec3?

@jvlomax
Copy link
Contributor

jvlomax commented Sep 29, 2014

I don't think we should worry too much about Create_wall, Create_house,
Create_massive_castle_on_a_hill. If we just give people the tools, they
will build

any position in the game world should be passed as vector3. Not sure
which is more pythonic, but it's a game, so vector3 makes more sense

On 29. september 2014 12:05:10, Ben Nuttall wrote:

Great. I think a compatible API that works in Python 3 is important,
and an extended version that's more Pythonic and more feature rich
would be ace.

Low level & high level sounds great too.

I think abstraction is good but don't want to rely on it. Functions
like |build_house| should be basic, not full solutions. The important
thing is to encourage learning and creativity, not to make a labour
saving device for gamers. For example:

def build_house(x, y, z, width, height, block):
"""
Builds a house at location (x, y, z) size width x height made of block type block
"""
...

This might just make a hollow cube with walls and a roof, with a door
at the front. The code would be simple enough that the user could
understand it and make their own function that did something else.

Thought occurs: is it more pythonic to pass in the location as a
3-tuple? Or Vec3?


Reply to this email directly or view it on GitHub
py3minepi/py3minepi#10 (comment).

@bennuttall
Copy link
Member

Functions like that shouldn't be the goto for main usage, just quick tools for getting started and demonstrating.

@ghickman
Copy link
Member

It would be great to have these examples available for examples/teaching so maybe we should include them in their own package?

Example:

from minecraft.shortcuts import build_house

@jvlomax
Copy link
Contributor

jvlomax commented Sep 29, 2014

+1

On 29. september 2014 12:25:44, George Hickman wrote:

It would be great to have these examples available for
examples/teaching so maybe we should include them in their own package?

Example:

|from minecraft.shortcuts import build_house
|


Reply to this email directly or view it on GitHub
py3minepi/py3minepi#10 (comment).

@ghickman
Copy link
Member

They could even serve as examples in the documentation for users/teachers where we show how to build up a build_house (et al) from the components provided in the main library.

@bennuttall
Copy link
Member

Sounds good

👍

@dbrgn
Copy link
Member

dbrgn commented Sep 29, 2014

I'd just keep the examples in the docs, or in a samples/ directory. Then people have to actively copy them to their project if they want to use them. This encourages learning over simply using things.

Regarding the vectors, I already commented on this in #14, I'd actually pass in flat variables (i.e. three arguments called x, y and z) instead. Or a tuple. I think vectors add to the learning curve but are not necessary at all.

If you still want to use vectors, rename them to something more sensible like Position. Also, we'd need both 2d and 3d positions (e.g. getHeight currently wants x and z coordinates and returns the y coordinate). Which means that you cannot just pass in the vector of the player. Instead, you convert the Position3D to a Position2D and then get back an integer. WAT? That will become a mess quite quickly.

If, on the other hand, you always work with integers and floats, that is easy to understand for a beginner. If have a function called get_height(x, y) and one called set_block(x1, y1, z1, x2, y2, z2, block_type), everyone will probably understand right away how to call it. But explaining set_block(start_vector, end_vector, block_type) is harder.

I know that this doesn't look beautiful to a software engineer. But always assume that you'll have to explain this API to a child that has never programmed before :) (I actually will next month, so that's why I'm pushing for an API that's as simple as possible.)

@ghickman
Copy link
Member

@dbrgn – There are two things here, samples being quickly available to users/teachers and code location.

These samples are going to be very useful to have on hand when teaching and it would be nice for users to be able to import working code (ie that we've tested). This is all about lowering the barrier to entry for the library and having to copy/paste code from somewhere that may or may not work isn't going to help this goal.

We shouldn't assume the repo/GitHub will be the main distribution point for the codebase given it is likely going to be packaged for the Raspbian repos too.

You've made some good points about the use of vectors vs arguments but could we keep that in #14 or start a new issue so things don't get too muddled please.

@doismellburning
Copy link
Member

+1 for keeping "samples" in the code, albeit skeletal samples as per @bennuttall

@dbrgn
Copy link
Member

dbrgn commented Sep 29, 2014

OK, I agree about tested samples. If they're called samples instead of shortcuts that should already clear things up.

@ghickman
Copy link
Member

@dbrgn – Yes, that's a good point.

@bennuttall
Copy link
Member

Samples 👍

x, y, z 👍

@jonathanfine
Copy link
Contributor Author

This issue is Low level APIs and there was much support for my proposal. It has now become a discussion of where to put sample files.

As a result, I don't know what the design decision tag is supposed to refer to.

Please could we all be more careful about this - you can always open a new issue if you wish.

@hashbangstudio
Copy link
Contributor

Added new issue #40 for discussion of location of samples

@dbrgn
Copy link
Member

dbrgn commented Oct 2, 2014

OK, so here is my proposal regarding the low level pythonic API. I think it should stay very close to the Minecraft Pi API, while at the same time being clean and easy to use. But without too many abstractions, so we should prefer working with basic data types instead of creating our own. That's something that can go into the higher level API.

Imports

I think the low level API should reside in a subpackage. A few suggestions for possible names:

  • minecraft.core
  • minecraft.lowlevel
  • minecraft.simple
  • minecraft.base
  • minecraft.basic

All important objects / types / functions should be importable directly from that package.

from minecraft.core import Minecraft

Connecting

There's a single class that can be instantiated. It should immediately connect to the server and keep the connection alive. The low level connection should be available at ._connection or ._conn. It should provide kwargs to customize the connection details like IP or port.

mc = Minecraft()
another_mc = Minecraft(host='192.168.1.10')

There should be no global state at all. Everything should be accessible via that instance.

Namespacing

I'd suggest a simple system of namespaces, without having to instantiate any additional things.

  • world
  • chat
  • player
  • camera

Function definitions

For the low level API, I'd go with flat arguments, with no position type. IIRC there was a lot of support for this in other discussions.

The block types should just be integers (or alternatively enums). Coordinates are integers or floats (player position).

world

  • mc.world.get_block(x, y, z)
  • mc.world.set_block(x, y, z, block_type)
  • mc.world.set_blocks(x1, y1, z1, x2, y2, z2, block_type)
  • mc.world.get_height(x, z)
  • mc.world.save_checkpoint()
  • mc.world.restore_checkpoint()

chat

  • mc.chat.say(message)

player

  • mc.player.get_tile()
  • mc.player.set_tile(x, y, z)
  • mc.player.get_pos()
  • mc.player.set_pos(x, y, z)

camera

  • mc.camera.set_normal()
  • mc.camera.set_third_person()
  • mc.camera.set_fixed()
  • mc.camera.set_pos(x, y, z)

Exceptions

We should have a sensible system of exceptions, but keep them minimal. Connection related issues should be solved separately in the connection class if possible (e.g. automatic reconnecting). If all fails, then we can throw an exception.

Right now the following possibilities come to my mind:

  • ConnectionError (if something goes wrong with the connection)
  • APIError (if the API raises an error or crashes, or something like that)
  • ValueError (for validation)
  • TypeError (for basic type checking)

Sample implementation

Here's a basic implementation of parts of the suggested API: https://github.com/coredump-ch/pyminecraft/blob/master/pyminecraft/minecraft.py It could be used as a starting point.

The connection class is based on the mcpi one. It could still be improved and extended.

@ghickman
Copy link
Member

ghickman commented Oct 2, 2014

@dbrgn – This is great! I'm really happy with the API you've laid out here.

One thing I'd suggest is switching minecraft.chat.post() to minecraft.chat.say(), I think that's closer to how people will see the purpose of the method. What do you think?

@dbrgn
Copy link
Member

dbrgn commented Oct 2, 2014

One thing I'd suggest is switching minecraft.chat.post() to minecraft.chat.say(), I think that's closer to how people will see the purpose of the method. What do you think?

Absolutely :) I updated the post above.

@dbrgn
Copy link
Member

dbrgn commented Oct 3, 2014

So, any suggestions regarding the naming of the package?

Some suggestions:

  • minecraft.core
  • minecraft.lowlevel
  • minecraft.simple
  • minecraft.base
  • minecraft.basic

I don't really like any of them, but think I'd go with core.

As soon as this is sorted out I can do a pull request.

@doismellburning
Copy link
Member

If I had to pick any of those it'd be core

@ghickman
Copy link
Member

ghickman commented Oct 3, 2014

+1 to core here.

@dbrgn dbrgn linked a pull request Oct 3, 2014 that will close this issue
@jonathanfine
Copy link
Contributor Author

@dbrgn
Thank you for sharing your code. I think we need a fresh start for the new low-level API, and find that your code shares too much with the legacy API. In particular, it relies on and is shaped by the legacy connection module. I find that module deeply flawed - see #47 Legacy API does not detect obvious errors.

Your code is an improvement, in that it checks the number of arguments. But there's more to be done.

@ghickman
Copy link
Member

ghickman commented Oct 3, 2014

@jonathanfine – While I agree a fresh start might be nice, I don't see any benefit for us here. The legacy code isn't completely terrible and while the connection class isn't perfect it's a good start that can be improved upon. As you said, @dbrgn's code is a step in the right direction so I would favour continuing in that direction and improving the connection class.

@dbrgn
Copy link
Member

dbrgn commented Oct 3, 2014

I think the closer the low level API is to the TCP protocol, the better. It should be simple and stable. Not too many abstractions.

@dbrgn
Copy link
Member

dbrgn commented Oct 3, 2014

Regarding the connection module - absolutely, it needs to be improved. This is mostly a copy from mcpi, to get the codebase to work. It's not something I designed or redesigned.

@jonathanfine
Copy link
Contributor Author

I've written something that does the right things for me. See #48 Please review lowlevel API code for URL.

See #44 Minecraft Low Level API: First draft for review for @dbrgn's version.

@dbrgn and others.
This issue is already getting a bit long. How about we put a summary of the discussion on wiki, and then close this issue. This will recognise the progress we have made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants