-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
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 |
There's no "table widget", you'd need to place tiles with border textures in a grid. |
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 (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: @miozune Ok I see, I didn't realize you could put other widgets inside |
There was a problem hiding this 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over debug log
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:
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:
chance per time
column valuable? Should I merge asteroids with the same name, or add(UIV)
etc indicators to differentiate identical looking asteroids?snake_case
vscamelCase
and spotless doesn't put me in line)?SpaceMiningRecipes
or remove thehashCode
/equals
overrides inIG_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)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 themrunClient17