BME-171: Waystone Integration #91
Conversation
| implementation fg.deobf("curse.maven:$waystones_project_id:$waystones_file_id") | ||
| implementation fg.deobf("curse.maven:$balm_project_id:$balm_file_id") |
There was a problem hiding this comment.
Correct me if I'm wrong, but I think these should be compileOnly not implementation, same as for Rubidium/Embeddium?
(Same for the FTB dependencies above, but that's existing problems so up to you if you wanna fix for everything or just for your change)
| * every time a waystone is added/updated and contains a list of all the current waystones. | ||
| */ | ||
| private void onKnownWaystonesEvent(KnownWaystonesEvent event) { | ||
| var waypointClient = WaypointServiceClient.instance(); |
There was a problem hiding this comment.
Please avoid using var unless there's a good reason to. Java is a typed language and works best when types are declared correctly!
| .ifPresentOrElse(g -> updateWaypoint(g, waypoint), () -> { | ||
| // Add the waystone group if it didn't exist before | ||
| // This is so it doesn't need to be a default group (if waystones isn't installed) and | ||
| // this is the easiest place to add it |
There was a problem hiding this comment.
Do you need to do this check for every waystone present in the event, or can you do it just once before the outer loop an avoid duplicated work?
| waypointGroups.stream() | ||
| .filter(g -> g.type == WaypointChannelLocal.GROUP_WAYSTONE) | ||
| .findFirst() | ||
| .ifPresentOrElse(g -> updateWaypoint(g, waypoint), () -> { |
There was a problem hiding this comment.
Nit: While short to write, this isn't a particularly performant way of doing this as you're having to:
- Check every node
- Then go back and find the first element
- Then do a check to see if that first element exists.
It also unnecessarily creates a new list, which adds to memory usage and so GC events.
While not as fancy looking, it's better speed and memory efficient if you just did a normal for loop to check each element. That way, you can short circuit as soon as you've found a match, meaning fewer elements are checked in the average case, and you're not creating any unnecessary objects that'll need to be GCed later 😊
It's not a big deal here as the numbers we're talking about are very small. But it's a good habit to get into when writing high-performance code, such as code that runs in real time on the main game thread.
| * @param group The waypoint group to add to. | ||
| * @param newWaypoint The waypoint to add/update. | ||
| */ | ||
| private void updateWaypoint(WaypointGroup group, Waypoint newWaypoint) { |
There was a problem hiding this comment.
This helper function might make more sense to put on the WaypointGroup class rather than this one, as it seems to not be Waystones-specific. That way we can reuse this method elsewhere 😊
| } | ||
| }); | ||
|
|
||
| waypointsToRemove.forEach((waypoint, group) -> group.remove(waypoint)); |
There was a problem hiding this comment.
Why put them in a map to delete rather than just delete them then and there? It's extra work you don't have to do and an extra data structure that needs to be GCed.
If it's because that'd be changing a data structure in place, I believe there are iteration methods that are more forgiving for that.
2 participants
Depends on #90
Adds a waypoint group for waystones with a custom icon for waystones. Waypoints for discovered waystones get automatically added and deleted. This should work without Waystones/Balm being installed (I tested it).