Skip to content
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

Xaero's World Map Compatibility for Train Map #8109

Open
wants to merge 7 commits into
base: mc1.20.1/dev
Choose a base branch
from

Conversation

JensenJ
Copy link
Contributor

@JensenJ JensenJ commented Mar 25, 2025

Hi There!

I really liked the new train map system but wanted it for Xaero's World Map and saw quite a few requests in Discord for it. So here is a very nearly done WIP version.

Here is a screenshot showing it working in a very basic test network:
image

There's a few things that I need to ask about before I mark this as ready to merge.

  1. Xaero's map doesn't seem to have any sort of API. So, in terms of compatibility, if something were to change in a later version I imagine a crash would occur. IThundxr mentioned in Discord I should put anything like this into a try catch (I haven't implemented this yet) that way it won't crash etc. But as the implementation uses mixins I am not terribly sure how to best handle this. Any pointers here would be greatly appreciated.

  2. Xaero's map pauses the game in singleplayer while the map is on screen. Regrettably, this means the packets for updating the train positions on the map do not seem to update as the game will not send them until the menu closes again. This means that when you open the map, you see the positions of trains when you last closed the map or in some cases trains don't render at all. This integration works perfectly as it currently is in multiplayer and when the singleplayer world is opened to LAN. I have tested adding an inject mixin to the head of the isPauseScreen() method within the Screen class within Minecraft and then using cir.SetReturnValue(false) if the screen is an instance of Xaero's base screen class. This makes the implementation work perfectly in singleplayer, but it modifies behaviour within Xaero's map. I am unsure of any other way around this. Is this an acceptable mixin to implement or should I look for another way? If another way is needed, I would appreciate any suggestions.

Feel free to request any changes or ask any questions. Thank you.

JensenJ added 5 commits March 24, 2025 00:17
This implementation follows the same style as the existing Journeymap integration.
There are a couple bugs with this that will hopefully be fixed soon.
Rendering order - The train map renders over all UI elements which are part of the map. This is due to the mixin targeting the tail of render(). However, targeting earlier in the method causes visual errors as the rendering of map sections seems to be multithreaded.
Trains disappearing - When another menu is accessed such as the map settings, the trains on the map seem to disappear for a while. Updating them in the world seems to make them appear again.
Xaero's map allows you to view maps of other dimensions the player is not currently in, so this fixes the wrong train map being visible when viewing maps from other dimensions. When ticking, we check if Xaero's map is open before ticking the rendered dimension as the rendered dimension is never unset outside of Xaero's render function. This causes issues if another map mod is loaded and tries to view the train map as it would tick whatever map Xaero's was last rendering.
This fixes the train map rendering over the top of UI elements in Xaero's. For some reason, the trains and stations still render over vanilla drawn text in the menu, but this is consistent with how the JourneyMap integration renders.
Xaero's map can layer multiple transparent screens so checking if the map is open isn't best performed by checking if the current screen is GuiMap. This fixes the map not updating if another menu (such as map settings) is open over the map. We still check if it's GuiMap when checking for mouse clicks on the toggle widget, as if another screen is rendering over it, there may be a button conflict.
This seems much simpler, I don't know why my first thought was to use an accessor before.
@JensenJ JensenJ marked this pull request as ready for review March 25, 2025 15:52
@JensenJ
Copy link
Contributor Author

JensenJ commented Mar 25, 2025

Okay, other than the two questions up above. I think this is ready for review.

@IThundxr
Copy link
Member

  1. for the mixins you can set require to 0 so they do not crash if they fail to inject
  2. a inject into isPauseScreen only when xaeros world map is open is fine

Xaero's world map pauses the game in singleplayer when open which prevents the train map from getting the train positions properly. This adds a mixin which overrides this behaviour by injecting into the head of isPauseScreen(). This allows the map to work perfectly in singleplayer.
Also the mixin for the map now has require = 0 incase of future changes which break the injection.
@VoidLeech VoidLeech added the pr type: feature PR adds a new feature or changes an existing feature label Mar 25, 2025
Switched to using an accessor instead of shadow fields, as this apparently is less prone to failure if Xaero's changes field names/types, as the calls to the accessor are wrapped in a try-catch.
Put try-catch blocks around any reference to Xaero's code incase of changes. Currently if an exception is thrown an error log is printed once.
Improved the IsMapOpen() function to check if the parent screen is actually xaero's map as I didn't realize you can access the map settings screen for example without the map being open. The game pausing override injection uses this logic now also for consistency.
@JensenJ
Copy link
Contributor Author

JensenJ commented Mar 27, 2025

Okay, I've implemented those 2 things and the try-catches around Xaero's code so I think this is now complete. Let me know if anything else will need changing or if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr type: feature PR adds a new feature or changes an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants