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

Make Ship instances persistent. #80

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@timjolson

timjolson commented Oct 23, 2018

Ship objects being replaced between turns caused problems for myself, and others. For anyone new to python, this update could save a lot of debugging time.

Changes:
Ship class tracks which id's have been used and retrieves the instances when needed instead of creating them new each turn.

timjolson added some commits Oct 23, 2018

Overwrote __new__ for Ship class
A Ship instance will be re-used and updated by Ship._generate, instead of creating new instances.
Old way:
Ship(1,2,3,4) == Ship(1,2,3,4) -> False
Ship(1,2,3,4) is Ship(1,2,3,4) -> False

New way:
Ship(1,2,3,4) == Ship(1,2,3,4) -> True
Ship(1,2,3,4) is Ship(1,2,3,4) -> True
@snaar

This comment has been minimized.

Contributor

snaar commented Oct 23, 2018

Concerns:

  1. Need to update all the other kits do make them consistent with this. Or at least the core of Java, C++, JavaScript, Rust, C# (still pending review).
  2. This doesn't seem to handle ships dying. Someone can be tempted into iterating over the ships array and cause invalid moves to be emitted and that also would be hard to debug.
@snaar

This comment has been minimized.

Contributor

snaar commented Oct 23, 2018

Oh, looking at it again, the 2) is not actually a concern, I misread original. I'm not very familiar with Python, but this will actually work correctly, with ships disappearing, right?

@snaar

This comment has been minimized.

Contributor

snaar commented Oct 23, 2018

@lidavidm what do you think re 1)? I'm ok updating the other kits to have similar behaviour. It's certainly a lot more friendlier.

@lidavidm

This comment has been minimized.

Contributor

lidavidm commented Oct 23, 2018

Mutable state ☹️

I'm fine with this; it'll make it friendlier. I would appreciate a comment there explaining what it's doing, for anyone reading the source.

Show resolved Hide resolved starter_kits/Python3/hlt/entity.py Outdated
@timjolson

This comment has been minimized.

timjolson commented Oct 23, 2018

Instead of overriding __new__ (which can be confusing and looks scary), a beginner-friendly-er solution might be changes to _generate instead:

def _generate(player_id):
    """
    Creates an instance of a ship for a given player given the engine's input.
    If an instance with the same ship.id has previously been generated, that instance will be returned.
    :param player_id: The id of the player who owns this ship
    :return: The ship id and ship object
    """
    # Read game engine input
    ship_id, x_position, y_position, halite = map(int, read_input().split())

    # Check storage to see if ship already exists
    # If the ship exists, update its position and halite
    if ship_id in Ship.__ships.keys():    
        old_ship = Ship.__ships[id]
        old_ship.position = Position(x_position, y_position)
        old_ship.halite_amount = halite
        return ship_id, old_ship
    else:
        # Otherwise, create and return a new instance
        new_ship = Ship(player_id, ship_id, Position(x_position, y_position), halite)
        Ship.__ships[ship_id] = new_ship
        return ship_id, new_ship

Advantages to this:

  • Other instance attributes can be added in __init__ rather than new (e.g. a mission, or a name).
  • An instance can be copied and manipulated without affecting the original.
    copy_of_ship = copy(ship))

Disadvantages to this:

  • Without using _generate (for example, importing and creating instances without a game running), Ship(1,2,3,4) == Ship(1,2,3,4) -> False
    and
    Ship(1,2,3,4) is Ship(1,2,3,4) -> False

Note that these should both work when using _generate/update_frame/etc.

If someone else could test these cases as verification, I think it's a better solution.

As for the state (__ship) being mutable, I haven't come up with an improvement.

timjolson added some commits Oct 23, 2018

@snaar

snaar approved these changes Oct 24, 2018

@snaar

This comment has been minimized.

Contributor

snaar commented Oct 24, 2018

Let us think about it for a bit. I think we like this, but want to check other kits first and also docs.

Merge pull request #1 from HaliteChallenge/master
Merge updates from Halite-III
fix id used as key in __ships when re-using Ship instance
Co-Authored-By: timjolson <timjolson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment