Skip to content

Track structure controller position and add structure sockets#50

Open
RecursivePineapple wants to merge 12 commits intomasterfrom
structure-sockets-and-controller-pos
Open

Track structure controller position and add structure sockets#50
RecursivePineapple wants to merge 12 commits intomasterfrom
structure-sockets-and-controller-pos

Conversation

@RecursivePineapple
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

sure, I'll add some tests tomorrow

@RecursivePineapple
Copy link
Copy Markdown
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
Copy link
Copy Markdown

@Luca-Guettinger Luca-Guettinger left a comment

Choose a reason for hiding this comment

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

1 issue with getControllerPosition not copying the internal Position (unlike the socket getters), plus 3 minor nits on dead code and a subtle behavior change.

@Override
public Position<StructureDefinitionCoords> getControllerPosition(String piece) {
return controllers.get(piece);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getControllerPosition returns the internal mutable Position reference. getSocket and getAllSockets both call .copy(), but this doesn't. Since the coordinate system translators mutate positions in place, any caller that transforms the returned position will corrupt the StructureDefinition's internal state. Should return controllers.get(piece).copy() (with a null check) like the socket methods do.

char[] chars = e.getValue().toCharArray();
IStructureElement<T>[] compiled = new IStructureElement[chars.length];

int a = 0, b = 0, c = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

a, b, c are tracked here but never read. Dead stores - the whole isNavigating branch can be removed, the method only needs to map chars to elements.

}
}

if (map.put(e.getKey(), compiled) != null) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This check is unreachable. shapes is populated from a HashMap, duplicate keys are impossible.

* @return this builder
*/
public Builder<T> addShape(String name, String[][] structurePiece) {
uncompiledShapes.put(name, structurePiece);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Deferred compilation means getShapes() now returns an empty map until build() is called. Before this change it was populated during addShape(). Probably nobody depends on this, but it's a subtle behavior change on a public method.

@Dream-Master Dream-Master added ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version and removed 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta labels Apr 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants