-
Notifications
You must be signed in to change notification settings - Fork 20
Track structure controller position and add structure sockets #50
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
base: master
Are you sure you want to change the base?
Conversation
| var piecePositions = socketLocations.get(piece); | ||
|
|
||
| if (piecePositions == null) { | ||
| throw new IllegalArgumentException("Invalid piece: " + piece); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
piece might exist but have no socket. throwing an error would likely cause some headache in downstream repos that needs to introspect a structure e.g. blockrenderer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant to be a "assert one and only one socket" type of method. getAllSockets only throws when the piece is missing, but I consider that a logic error regardless of the usage. If something is dynamically scanning for sockets, it should check if the piece exists first (though it doesn't look like that method exists, should I add it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't getAllSockets throw when piece have no socket either? you did not eagerly create an entry in socketLocations for every piece. you did so only when it has a socket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, multimaps return an empty collection for keys that don't exist. Since it's a ListMultimap it'll return an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final Map<String, ListMultimap<Character, Position>> socketLocations = new HashMap<>();
socketLocations is a HashMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data is roughly {piece name: {socket char: Position[]}}. The outer piece -> socket map is a hash map, but the socket char -> position map is a multimap. I know it's an ugly unreadable type but I can't think of a way to improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to convey this better but let's just say this piece of code will throw with an exception saying main is invalid
builder().addShape("main", new String[][] {{"~"}}).build().getAllSockets("main")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out what you meant, it should be good now. I added some tests to validate that it's correct.
Glease
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a full review. just a few thoughts from quickly skimming through the changes
perhaps a few tests as an example of how to use the CoordinateSystem and its subclasses? I know we could always just look at the gt multi that uses it as a reference, but gt5 is quite a volatile repo and things will change without either of us know it.
|
sure, I'll add some tests tomorrow |
|
It took me a bit longer than I expected because I had to defer the shape compilation into |
This does three things:
There are new query methods for controller and socket positions.
The goal of this PR is to reduce the amount of hardcoded numbers in structure implementations.
I tested this in another GT5u PR which should be going up soon. I made sure that multiblocks produced the correct world position for every orientation of a controller.