KingContaria / seedqueue

Minecraft mod that generates multiple worlds concurrently and lets players reset them on an ingame wall screen.
MIT License
32 stars 18 forks source link

feature: add the auto option for background previews #62

Closed nealxm closed 2 months ago

nealxm commented 2 months ago

a start on resolving #46

for all the other configurations, 0 is used for the auto state. currently this follows that convention for consistency. but i was wondering if 0 might be a state you want to preserve since having 0 background previews may be desirable for lower end hardware.

tildejustin commented 2 months ago

you could make min = -1 to use for auto, although then you can't use the field (but it seems to be only used for thread priority anyway)

nealxm commented 2 months ago

-1 made sense to me too yeah. the only issue i found with that is the annotation above the backgroundPreviews declaration is for whole numbers only. is there another annotation for integers? i wasn't able to find it

tildejustin commented 2 months ago

it's just an int, long, and short wrapper, I'm pretty sure it works for negative numbers... https://github.com/KingContaria/SpeedrunAPI/blob/master/src/main/java/org/mcsr/speedrunapi/config/option/WholeNumberOption.java. maybe best to ask contaria.

nealxm commented 2 months ago

i see, thanks for the help! you're right it works fine subbing -1 for 0. my fault i didn't think the whole number annotation would take integers

Crystal15118 commented 2 months ago

i was working on it 🙄

Crystal15118 commented 2 months ago

This branch has everything i've done so far

KingContaria commented 2 months ago

Using rows x columns is not a reliable way of getting the main groups size, since resourcepacks can override that. Instead SeedQueueConfig#getBackgroundPreviews could return an Optional and if it's set to AUTO it returns Optional#empty. This would then tell the wall screen to calculate the background previews using the data available through SeedQueueWallScreen#layout. That would also allow for including the size of the preparing group in the formula like i suggested in the issue

KingContaria commented 2 months ago

Also justin is right, -1 will work

nealxm commented 2 months ago

thanks for your feedback! was working on this a bit with @Crystal15118 and i think this is a step in the right direction. i think it can be done with only SeedQueueWallScreen#getBackgroundPreviews without the use of Optional & SeedQueueConfig#getBackgroundPreviews but i wanted to include that since was suggested

nealxm commented 2 months ago

i added the new process for getting the sensible value and also cleaned up the rest of my edits so it doesn't require the use of Optional anymore. hope this is better!

KingContaria commented 2 months ago

Lgtm, only thing left to discuss is if the option should be renamed internally to reset all users to AUTO, opinions are welcome

KingContaria commented 2 months ago

Please rename backgroundPreviews (just the field, not the method in SeedQueueWallScreen) to reset everyones option to Auto. I'd recommend preparingPreviews just because it's consistent with the preparing group in custom layouts.

nealxm commented 2 months ago

should be good to go!