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

Add asteroid summary and cache/improve recipe lookup for space mining #76

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hacatu
Copy link

@hacatu hacatu commented Sep 22, 2024

This PR adds summary text to the space miner that says what asteroids it might mine at the current distance, their chance per operation, chance per time, and max parallels. It also improves recipe lookup, so instead of using default gt recipe lookup and filtering by distance and tier, it computes what drones can currently work and then uses basically an interval tree to look up the possible asteroids. This interval tree is automatically computed once at startup (total time during loading is 0.1 seconds, but it should reduce calculations in game).

Example of summary screen:

UIV-121
UHV+UIV-121

Is this format good? I decreased the text size so it doesn't have a ridiculously large scroll bar but it still looks a bit weird.

It does not merge identical recipes from different drones (I have to explain how drones actually work for this to makes sense).

The space miner does not lock to one drone tier and process random asteroids until it runs out of inputs, then switch to a different drone tier if possible like you might expect. Instead, it combines all the possible asteroids from all the drones with available inputs and picks a random one from the whole list. I preserved this behavior in case anyone depends on it and isn't just using dedicated miners per drone.

This does mean that the summary sometimes contains multiple copies of some asteroid like "CHEEEESE!", if multiple available drones can get it, and it might not be clear to users why this is.

The new recipe lookup worked perfectly when I tested it in runClient17, but I have NOT tested it in the full pack.

There are a few things I'd like feedback on:

  • Is the summary format good? Is it clear what the numbers mean, and is the chance per time column valuable? Should I merge asteroids with the same name, or add (UIV) etc indicators to differentiate identical looking asteroids?
  • Is the variable naming ok (I'm a bit inconsistent with snake_case vs camelCase and spotless doesn't put me in line)?
  • Should there be more comments?
  • Should I move some of the methods from SpaceMiningRecipes or remove the hashCode/equals overrides in IG_SpaceMiningRecipe? (These comparisons are considerably more expensive than default comparisons and should not be needed with the new interval tree based recipe lookup, and even if they were needed, mandating uniqueness of recipe outputs per asteroid name and recipe inputs per drone would make these comparisons not expensive)
  • Is the use of new features like records ok? Existing code uses some features like if ... instanceof and lombok features (well other mods use lombok features), but records don't really provide that much utility and some people might not be familiar with them
  • I would like someone else to confirm it works because I'm worried I forgot some edge case even though it works in my testing in runClient17

@Dream-Master Dream-Master requested a review from a team September 22, 2024 17:41
Copy link
Member

@miozune miozune left a comment

Choose a reason for hiding this comment

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

  • While I appreciate the effort you put into optimization, I'd argue that it should not be included for these reasons:

    • Simply caching the last settings will improve performance significantly, as players won't change settings frequently.
    • Total number of recipes is merely 225. I think that at this number you don't know if this tree is actually faster than naive loop unless you profile the game.
    • Optimization is usually trade-off with maintainability. Not many people can read this tree code, and there's no guarantee you would be available when someone wants to add new machanism which requires modification to the lookup logic.
  • As for the GUI, it's ok-ish, but it would be great if you add some sort of table to make it less pixelated. You don't need to stick to the "base" GUI design. But if it's hard to implement that's fine, MUI2 will provide easier way to layout such a thing in the near future ™️

  • We want camelCase, though we're not strict.

  • I heard record doesn't work with Forge transformer and am not sure if @Desugar works. Better to check with full modpack with Java 17 or something.

@hacatu
Copy link
Author

hacatu commented Sep 29, 2024

Thanks for the feedback

Ok, I mostly split the changes into two commits with the interval tree recipe lookup changes being in the second one, so it should be pretty easy to remove

In theory, the complexity of the code isn't an issue because it automatically transforms the recipe map, but like you said it doesn't allow new kinds of space mining recipes that don't use drones or use different secondary items like saws or something instead of drills. And 99% of the performance gain comes from just caching the recipes

I'll try to check I'm using camelCase and remove the records

I can make sure the columns in the summary are aligned, but I don't know how to make a new table widget for mui1. I would think ASCII art tables cuz then it still gets the scroll bar from the text widget for free, but I don't think the text widget supports underlined text or some of the other stuff that would need. You can message me in mod-dev on the discord if you have suggestions about this

@miozune
Copy link
Member

miozune commented Sep 30, 2024

There's no "table widget", you'd need to place tiles with border textures in a grid. drawTexts should accept any widget as a child, but I'm not sure if it actually works good.
I mean current design is not terrible imo, though I'm not a endgame player.

@hacatu
Copy link
Author

hacatu commented Oct 1, 2024

I removed the interval tree recipe lookup, records, and snake_case variables

I also noticed a bug where drones were considered "available" even if the drone wasn't present, because the DRONE_STACKS has itemstacks of size 0 (I didn't write that and expected them to have size 1)

(BLAH BLAH BLH: with the now removed interval tree recipe lookup this would have been a huge issue since it would actually do recipes without the necessary drone. However, with the default recipe lookup restored, it would just cause the cache to not be updated (and continue doing recipes with a drone even after it was removed, but it would have to be present at some point))

(TLDR: DRONE_STACKS had itemstacks with size 0 so my logic to get the drones which had the items to run a recipe was flawed, but it's fixed now)

@miozune Ok I see, I didn't realize you could put other widgets inside drawTexts (even though the code does do that already oops). I still don't know mui enough to write a custom widget unfortunately, and also aligning the text without a table widget is actually not simple because it's not a monospaced font. Plain text will have to be "good enough" for now

Copy link
Member

@miozune miozune left a comment

Choose a reason for hiding this comment

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

I don't find any other obvious code error. However, I don't have time to thoroughly inspect code, nor do I have enough endgame knowledge to verify if it works as expected, so reviews or test plays from other people are appreciated.

&& recipe.mSpecialValue <= tModuleTier)
.collect(Collectors.toList());
int availDroneMask = getAvailDroneMask(inputs);
GTNHIntergalactic.LOG.warn("availDroneMask: " + availDroneMask);
Copy link
Member

Choose a reason for hiding this comment

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

Left over debug log

@Dream-Master Dream-Master requested a review from a team October 24, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants