Skip to content

Conversation

@RecursivePineapple
Copy link
Contributor

This does three things:

  • Adds a statically check type-safe block position system, that allows you to move between coordinate systems easily and know your transforms are correct
  • Adds code that tracks controller positions (tildes) in structure shape definitions
  • Adds 'sockets,' which are essentially fake structure elements that record their position

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.

@RecursivePineapple RecursivePineapple requested a review from a team October 28, 2025 16:37
@RecursivePineapple RecursivePineapple added the enhancement New feature or request label Oct 28, 2025
@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Oct 28, 2025
var piecePositions = socketLocations.get(piece);

if (piecePositions == null) {
throw new IllegalArgumentException("Invalid piece: " + piece);
Copy link
Contributor

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

Copy link
Contributor Author

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?)

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@Glease Glease Oct 31, 2025

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")

Copy link
Contributor Author

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.

Copy link
Contributor

@Glease Glease left a 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.

@RecursivePineapple
Copy link
Contributor Author

sure, I'll add some tests tomorrow

@RecursivePineapple
Copy link
Contributor Author

It took me a bit longer than I expected because I had to defer the shape compilation into .build() to simplify the socket code, but everything should be working and tested now.

@Dream-Master Dream-Master requested a review from a team November 2, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants