Config stuff

[last iris]

btw, is there any differnce between ModList.get().getModContainerById(modConfig.getModId()).orElseThrow().getModInfo().getDisplayName() and mod.getModInfo().getDisplayName()?

[mystic temple]

random side question. Your backend rework of the config system didn't happen to fix this: https://github.com/neoforged/NeoForge/issues/5 did it tech?

[steady tiger]

why

it's their mods' client settings

[severe sorrel]

indeed

I thought the config screen might want to use it

[steady tiger]

it doesn't make sense to have mods override any of these

[steady tiger]

did it fix it or did it not

[severe sorrel]

I didn't change it

[steady tiger]

pup when you say fix you

're referring to

Given the fact that at some point support for config value caching got added it would make sense to only invalidate the cached values for entries that need a world restart when the config unloads (server is closed aka world restarts).
right?

[mystic temple]

more or less

the flag is entirely unused

currently

[steady tiger]

i can open a pr to "fix" it

it would also make sense to not clear caches of startup configs

[mystic temple]

so like modders can set it on things, and will probably incorrectly assume that it will make the config option only happen on world leave/join or server stop/start. But it doesn't actually prevent that at all

[severe sorrel]

why would they ever be cleared?

[steady tiger]

the config screen calling set

[mystic temple]

Or potentially file watcher, unless they are excluded from being added there

[steady tiger]

they aren't as far as i can tell

or they aren't as far as i can tell

[last iris]

just don't forget to add a second getter for the real current value so the UI can get it

[steady tiger]

and if they are updated with the file watcher, that sounds like a massive oversight lol

[steady tiger]

@severe sorrel can you confirm or deny this

[severe sorrel]

they are listened to by the watcher yes

[last iris]

btw, unless I overlooked something, editing and saving a startup config will fire a config reloaded event. that might be unwanted

[steady tiger]

[mystic temple]

which I guess is sort of partially needed for showing it in the config gui with the correct value?

[severe sorrel]

yes

[last iris]

true

[severe sorrel]

I don't really see a technical reason to restrict the modification of STARTUP entries

we might want to add a requiresGameRestart flag

[mystic temple]

will it be like the requiresWorldRestart flag that doesn't actually do anything? /s

[steady tiger]

yes duh

[last iris]

here, see those two identical buttons? That's why the modder should be told what the translation keys for them are

[steady tiger]

this is neoforge, we only do half the job

[steady tiger]

ok, but most mods will not have 2 identical buttons

[mystic temple]

if a mod has two identical buttons then log the warning in dev with the translation key?

[last iris]

btw, with the UI those two restart types now do something

[steady tiger]

if you want to tell modders what the config key is, document it 😛

but logging it when it doesn't affect them doesn't make sense

[mystic temple]

finally

1.12 loader feature functionality

[last iris]

pup, it already is guarded behind a config setting and a !FMLLoader.isProduction() check. No need for any more conditions. I'll just reinstate the translationchecker for those keys

[steady tiger]

please no seriously, don't make modders translate everything

that's against the point of reasonable fallbacks

[last iris]

but this fallback is not reasonable. it produces visually identical buttons for different configs

[steady tiger]

it is reasonable for those that don't have multiple configs of the same type

[severe sorrel]

can we enforce translations if there is more than 1 file of a given type?

[last iris]

why enforce them? There's a consolidated list of untranslated keys that is shown if (a) it is enabled in the config and (b) !FMLLoader.isProduction().

[severe sorrel]

shown where?

[last iris]

in the console. once when exiting the config.

[steady tiger]

and for the record, out of the 1.20.4 FC pack, exactly 3 mods have multiple configs per type

[last iris]

if it is enabled AND the game is running in a dev environment AND the modder visited his mod's config

[steady tiger]

configui.configuration.title is also a reasonable default

you're going a bit overboard with the amount of things you expect a modder to translate in a standard gui 😛

[severe sorrel]

seems fine to have a warning, but I'd not have the fallback at all in the GUI and just print the key in the GUI

[quick shuttle]

Does the config gui use the config comments directly without needing a translation key on them?

[last iris]

comments are used as fallback for tooltips

[steady tiger]

especially if the config gui will become default at some point

[severe sorrel]

I think that if you don't specify multiple configs of some type you should get a good default and no warning

and if you do have multiple configs of some type, I'd show a warning in the logs and untranslated keys in the GUI

i.e. provide a good default and only provide it when it makes sense

[last iris]

how about this?

[steady tiger]

hmmm

how should a getter for the actual config value should be called

[steady tiger]

i can't tell if you're joking

[last iris]

more venting than joking

[steady tiger]

maybe i shouldn't add one and instead have the config UI call getRaw

[last iris]

from the name it sounds fine, I'd have to check if there's more processing between get and raw

[steady tiger]

there isn't

get is a cached getRaw

also something weird i noticed, ConfigValue#getPath returns a new array list instead of an immutable view

that does not sound efficient

[severe sorrel]

without looking at the code: it doesn't matter

[lilac leaf]

I still want to fix config loading finishing too late on the client but there was no agreed upon solution

[severe sorrel]

ah right I can actually fix that

[steady tiger]

i hate boolean logic

[lilac leaf]

Oh ok, thanks

[steady tiger]

https://github.com/neoforged/NeoForge/pull/1284

[severe sorrel]

I don't see the point of that tbh

[steady tiger]

the point of what

[severe sorrel]

this PR

the way I see it, the flag is more of a player information than a technical thing

[steady tiger]

well consider a config value that is used to register something which obviously cannot be updated. now if a player updates the config and that config value is also used somewhere else then they'll see a mismatch

[severe sorrel]

that's true

on the other hand they'd see a mismatch in the config screen already

[steady tiger]

those values are not directly visible, and if anything the config UI can mark a value as outdated

making the config UI actually respect the flag already would need it to do that

[last iris]

the config screen already enforces those restarts. once you changed a STARTUP or needsWorldRestart value you cannot exit the config UI without exiting the game/returning to the main menu

[steady tiger]

or that

[severe sorrel]

hmmm that is annoying on the UX end I think 😄

[steady tiger]

it does warn you

[severe sorrel]

IMO a warning is fine as long as I can choose to continue to play

[last iris]

PR is updated with new screenshots and the extended test config

[steady tiger]

imo if you can choose to continue to play there's even more incentive to not update the value

[severe sorrel]

yes

[last iris]

at the moment the values are live-updated. and as long as that's the case, I insist on enforcing the restart. once they won't seem to change for the mod, I'll happily change the warning texts and replace the "back to config" button with a "back to game" one

[steady tiger]

for configs that require a restart the config ui can check if Objects.equals(get(), getRaw(...)) and display a warning in the tooltip or something

[severe sorrel]

I think the idea is sound

I'll take a closer look at the code somewhat later

[last iris]

I'll put that onto my list for that change

[steady tiger]

okay now i really hate boolean logic

Preconditions.checkArgument(context.needsGameRestart() == context.needsWorldRestart() && !context.needsWorldRestart(), "Cannot require both world and game restart"); this should make sense, right?

[last iris]

how about using the same enum I use in the UI?

[sacred rune]

context.needsGameRestart != context.needsWorldRestart

[steady tiger]

false == false

            if (context.needsGameRestart() == context.needsWorldRestart()) {
                Preconditions.checkArgument(!context.needsGameRestart(), "Cannot require both world and game restart: " + String.join(".", path));
            }

that should work

[severe sorrel]

I don't understand the purpose of that check

surely you mean Preconditions.checkArgument(!(gameRestart && worldRestart))

[last iris]

come on, just use that tri-state enum. juggling 2 bits for 3 valid states is just meh

[steady tiger]

I mean sure

builder.needsRestart(RestartType.WORLD).define?

[mystic temple]

Level

[steady tiger]

okay we can have LEVEL in code

but we should show the user world

[last iris]

also, is there any role on this server that gives an EnderIO-purple username but does nothing else?

[severe sorrel]

no 😛

[steady tiger]

i'm sure you'll appreciate the banned role

[last iris]

the setters on the builder can stay and do needsRestart = needsRestart.with(SERVER) internally

[severe sorrel]

ugly 😛

[lilac leaf]

Related to the PR you just opened, I think it makes more sense if common and client setup events are also done firing before the resource reload starts, because there are use cases for using those events as a "registration finished" callback, and it may be important for that callback to run before some custom reload listener

[severe sorrel]

that would be a large breaking change with unforeseen consequences

[lilac leaf]

The place where they start firing doesn't have to change, but all listeners just have to be done before the resource reload

I don't see how that would be a breaking change

[severe sorrel]

ah yes

well it is nice from a performance POV to fire these events in parallel I suppose

[lilac leaf]

It can still be parallel

Isn't there a dedicated async executor in FML?

Using the reload executors for mod loading is weird IMO anyway

[severe sorrel]

yes but it's even better to do it in parallel with the vanilla reload

I would rather add more events that target specific use cases

[lilac leaf]

I'm not convinced because on the dedicated server all mod loading is completely finished before the resource reload starts

If it actually had a performance impact, that would have been changed

[severe sorrel]

ermmm

that's not how things worked on Forge

[lilac leaf]

[severe sorrel]

our startup is slow

[lilac leaf]

I thought that was because of the classpath scanning or whatever and not because a couple events don't run in parallel

[severe sorrel]

yes but it's a general theme

I don't know

[lilac leaf]

I don't think enforcing an (in my opinion) bad event contract is worth saving a couple milliseconds on startup or making new separate events with a better contract

[severe sorrel]

I don't know

maybe open an issue

[lilac leaf]

That might help, sure

[last iris]

It might be worth considering adding a config type for datagen that's reading from a file somewhere in src that's not included in the jar. people might want to organise some of their data in a way that's not spread out in a mile of code.

[drowsy iris]

I don't know what configs you would want for datagen since the result of datagen will be published

[last iris]

not sure, but in the past I put almost everything that would have resulted in hardcoding a number or a string in either a config value or an enum (if there were enough similar objects). So I could see people wanting to do that. But otherwise it's just an idea I had when reading about the change in when common+client configs are loaded

[severe sorrel]

when are we adding requiresChunkRedraw to the options? 😄

[quick shuttle]

I launch the config gui with my mod

what the fuck am i looking at

[severe sorrel]

[quick shuttle]

please dont tell me each config button is hardcoded to only show the config type and not the file name...

[severe sorrel]

^

[quick shuttle]

this is how I do my configs for reference

are sections going to be forced to either be translated with space or have the config file show the key?

seems like I cant do proper key format for gui and spaces in config file at same time

[quick shuttle]

I cant figure out what the translation key is for these

[severe sorrel]

it might be in your logs

[quick shuttle]

i didnt see any. must not be in yet

[severe sorrel]

like that

[severe sorrel]

ah yeah maybe not yet

[quick shuttle]

Also, do sections themselves really need tool tips? The sections usually got a short few words that describe what they group up anyway. The individual config entries having tooltips should be enough

at the very least, section tooltips should be hidden if no lang entry for the section tooltip exists

Yeah... I rather not have a section tooltip. Cant seem to yeet it

Tooltip got cut off by top of screen lol
Should be going downward from my cursor. not upward when hovering on first config entry

[quick shuttle]

hmm nvm on this. Was a borked tooltip with excessive number of lines

[quick shuttle]

Do sliders really need to have text on them when they got a label next to them? I feel sliders should only have the number on top. No text on top

[mystic temple]

yeah should just be the number imo

[last iris]

That is what frenky and I have been fighting over for hours... ;)
(frenky, Remember how my version originally built that text from the filename for exactly this reason. Just as if had anticipated this happening for some mods... )

[last iris]

you should be able to set a translationKey for the gui and use whatever key you want for the file. The gui will use the translation key if it exists and (as far as I'm aware) the file will ignore it completely

[last iris]

those extra newlines are a bug. Or a forgotten TODO item to remove. The tooltip for the name is always there are the buttons are not very long and the text easily gets cut off (which isn't something the button tells me)
Edit: However, it's not quite this trivial. It's a translateable component that gets handed over to the button...

[last iris]

that's 100% vanilla behaviour

[last iris]

that's just how vanilla sliders work. I do override this for enumButtons, I'll have a look into what it takes to do that for sliders, too
Edit: huh, this was easy. Done. The slider has a callback for that, the enum button needs an AT and copying a whole class. Vanilla is inconsistent again...

btw, note that tooltip translations are optional and the comment will be used if there's none.

[last iris]

and got the missing tooltip case sorted out. If there is no tooltip translation and the comment is empty, the newline is omitted. If there is an empty tooltip translation (zero-length string or whitespace-only string) ... nope, that's what the modder ordered, I'm not doing string analysis on the translated texts.
However, may I state that not adding a comment to a config value is a bad practice in my opinion? For the modder, it may be perfectly clear what "Enable flubberdiwoop intermediate non-euclidean processing optimisation" means, but exposed config values are not for the modder.

[severe sorrel]

The line breaking is really weird, no?

[last iris]

?

[severe sorrel]

missin
g

[last iris]

that's just vanilla tooltip rendering. I do nothing but set the tooltip Components on those widgets

[severe sorrel]

I wonder if our automatic tooltip wrapping code is messing it up 😄

[last iris]

I wouldn't be too concerned. Words that are so long that they get broken up are quite rare

Or do we expect many mods to have settings like Donaudampfschifffahrtsgesellschaftskapitänsmützenaufbewahrungsschrankherstellungsgenehmigungseinstellung?

[severe sorrel]

Ja ja

[maiden comet]

you make me want to create a mod like this

[oak crescent]

well, that definitely can't be the modid

But the abbreviation of "Rinderkennzeichnungs- und Rindfleisch­etikettierungs­überwachungs­aufgaben­übertragungs­gesetz" would easily work

[steady tiger]

fun fact, bumblezone is one of those exactly 3 mods out of 160 and something that had multiple configs per type

so :P

[steady tiger]

also see, this is why the section buttons should be big

there isnt enough space for them not to be cut off

tech's suggestion seems sane to me, just show untraslated keys if there's multiple configs

[last iris]

which would mean additional code to first count the configs and then do a second pass doing different things based on that count. And what for? Any modder who opts their mod into the system would at least look at the config screen once, would see that there are two identical buttons, and could then see a nice message in the console telling them which keys to put into their lang files to change that.
We could just as well add some AI code in to produce names for blocks based on their texture...but we don't, we expect modders to supply those together with the rest of the mod. Block names and config screens are not something that gets 3rd-party-applied to a mod without the involvement of the mod maker. In fact, I find it much more likely to get a feature request to write that key list into the lang file instead of the log so that people don't have to copy it over manually than one that a modder wants an automatic name for their config files. People aren't exactly opposed to naming things that will be displayed to players. (They may not be good at it, sure, but that's another topic.)
If anything, I'd vote to add a translationKey as a mandatory parameter to the config registration method and use that. (or a ResourceLocation to mimic other registrations, those have a toTranslationKey() method that works fine.)

PS: I could also put the translation key into the tooltip when running in a dev environment for those people who don't look at the console. Then it'd be a bit more on the nose...

[steady tiger]

as I said before, reasonable defaults

for the majority of mods that do not have multiple configs per type you're going to have each mod provide even more translations for something that could have a reasonable default in neo

not only is that duplicated work for mod devs especially if the config screen becomes default which is the plan at some point™️

but it's also a negative for users because not all mods will have translations for all languages neo has

so if a reasonable default for my mod that adds 2 config values to one client config isn't provided

i'll have to translate Client configuration and mymod Client Config and whatever else could have a default

and if a user uses russian they won't see any translation

while if they had defaults they'd see the translation neo provides because people contribute translations to neo, not to a random modder with a few downloads that doesn't even want to deal with checking and managing translations

so unless you expect each mod to translate Client settings in 23 languages..

[quick shuttle]

What are the other 2

[steady tiger]

mek

and byg(?)

i haven't checked byg's code, just saw 2 worldgen toml files

[quick shuttle]

How did you check

[steady tiger]

i looked at the config folder

hmm wait not byg

byg isn't on 1.20.4

something worldgen related with a similar name

[quick shuttle]

You may want to do a massive 1.20.1 modpack to check. Not 1.20.4

[steady tiger]

i don't feel like installing 1.20.1 packs right now, but that won't get the number past 50% in any case 😛

[steady tiger]

bop

[drowsy iris]

how are you checking, just different suffixes than -client -common -server?

[steady tiger]

and after checking their 1.20.4 code (multiloader in 1.21 so they aren't even using neo configs) it was server+common and 2 server

[steady tiger]

yes, or well, files that aren't named -client/-common

[drowsy iris]

I'll just install ATM9, 422 mods 😛

[steady tiger]

yes have fun

didn't atm9 have connector too

[last iris]

why would expect this? Just by telling a modder what the translation ey is they can translate if they want instead of keeping it a secret to only be discovered by code diving?

[steady tiger]

If anything, I'd vote to add a translationKey as a mandatory parameter to the config registration method and use that. (or a ResourceLocation to mimic other registrations, those have a toTranslationKey() method that works fine.)

my response was related to that

[last iris]

oh, I see. read that as "to the 3-parameter registration method". The 2-parameter one can only be used once per type, so it's of no concern, it doesn't produce doubled buttons. The only issue with it is that the UI cannot tell its rsult apart from the 3-p one without doing some weird string guessing on the filename

[steady tiger]

tbh it would be nice if we could show the name of the config file in the button somewhere

[last iris]

    public void registerConfig(ModConfig.Type type, IConfigSpec configSpec) {
      registerConfig(ResourceLocation.of("neo", type.name().toLowerCase(Locale.ENGLISH)), type, configSpec);
    }
    public void registerConfig(ResourceLocation name, ModConfig.Type type, IConfigSpec configSpec) {
      registerConfig(name, type, configSpec, defaultConfigName(...));
    }
    public void registerConfig(ResourceLocation name, ModConfig.Type type, IConfigSpec configSpec, String fileName) {
      ...
    }

and then we use name.toLanguageKey("configuration") to get the button label

decoupling the name (translation key source) and filename on purpose here

we could add the actual filename (with full path?) in the tooltip, but so far the only tooltip on those buttons is the reason why they are disabled. This makes that stand out more

[quick shuttle]

If showing full path, just start the path inside config folder.

So configs/something/thing/pie.toml shows /something/thing/pie.toml

[last iris]

tbh, people who need the path need the full one. Those who can find the config folder on their own can also guess which file belongs to which button.

[quick shuttle]

Won’t you have a button already on config gui to open config location

[last iris]

but ideally, nobody needs to find the file as everything can be done in the gui

no, we don't. let the launcher handle that.

[quick shuttle]

Surprising number of people don’t use launchers. Just vanillas

And you stated you wanted to show full path for people to find the config location

[last iris]

then it'd be more of a thing for the main or mod menu. "Open instance folder", for logs, crashlogs and configs

[last iris]

that was more of an offering "if you think so" than a "I want this"

sorry, I'm so used to anticipate what customers want and offer it to them if it doesn't hurt the solution that I forget we usually only offer what we suggest here

[drowsy iris]

@steady tiger https://gist.github.com/shartte/85b121a00fc86b75d85db6f6fe20e417

That is from ATM9

[steady tiger]

https://gist.github.com/shartte/85b121a00fc86b75d85db6f6fe20e417#file-multiple-config-types-txt-L171-L173

wtf

raoul

[drowsy iris]

Yeah also noticed that, lol 😄

[quick shuttle]

lol atm still refuses bumblezone

[steady tiger]

so 12 mods

out of 400 something

[drowsy iris]

221 have configs registered

[quick shuttle]

anyway, look at the use case. It's smart and keeps configs from bloating one files to a ridiculous amount

god help player souls when they open a 10000 line config and have to find the line they want

[drowsy iris]

well the hope is they no longer have to with a UI 😄

i hope we add search

[quick shuttle]

@last iris do you have your newest changes up

[drowsy iris]

imagine not being able to strg+f since you have to open 20 files 😄

strg.... the german is showing

[last iris]

yes

[drowsy iris]

ctrl+f

[last iris]

any decent text editor has "Search in files..."

[quick shuttle]

i know users that didnt know

[last iris]

and btw, given the perceived numbers, I'd guess we could just have those few non-German devs learn German and switch the project language

[quick shuttle]

Config gui should have a search bar imo. Finds matching config entries that match in name first, then match in tooltip

just refreshing gradle should give me latest changes right?

[last iris]

that would be a major architecture change. ATM the UI doesn't scan the config but only sees the section each screen displays and has no way of creating the matching screen without cascading there through the screens on the way

[quick shuttle]

ok so mvp without search and add it later

[last iris]

yes, fast-forward and F5 (filesystem refresh), not even needs a gradle refresh if you already have the ATs

(mmmh, I do have an AT in there, I think? Edit: yes, I do)

[quick shuttle]

https://tenor.com/view/jfk-clone-high-i-like-your-funny-words-magic-man-jack-black-gif-18659433

fast-forward and F5 (filesystem refresh)

[last iris]

oh, you're pulling in the PR build into a mod, not running the neo branch. ok, then it's a gradle refresh (and I have to assume that PR build is up-to-date with the PR branch)

just one word of explanation: The design concept was to have the Ui be a shallow translator that converts the config into UI elements and maps changes back. So I don't build a shadow-tree of the configuration with my own objects (like <=1.12.2 did). That also means the UI can't even search through the current screen as it doesn't know about the UI elements or the config, other than going back to the source and iterating them again. For example, a text field is a vanilla widget and it directly writes to the ConfigSpec with a callback in its onChange and tells the Screen to remember to tell its parent to remember to save the config.

[drowsy iris]

The internals of the config UI itself are not API, yes?

So it's possible to change this later without BC?

[last iris]

The only kind of memory outside the currently displayed screen the system has is a small cache of sub-screens to be re-used so the undo buffer stays alive

[drowsy iris]

Should the need arise for searchability or otherwise

[last iris]

much of it is made to be subclassed. So major architecture changes would mean doing them in a copy and deprecating the old one until the next breaking phase

[drowsy iris]

Urghsl

I think search is kinda mandatory to have at some point. Not necessarily now, but later

[last iris]

btw, I personally am a fan of doing big changes as a "copy and deprecate the old still-working one" wherever it's possible. It makes live easier for both sides. And here it certainly is, as the whole UI is self-contained. There's not a single reference into the UI classes from outside, aside from the one that enables it for neo's config

[last iris]

also, what's the general opinion about this one? If it's positive, I'd look into how to make a fml PR for it

[steady tiger]

that's not possible

FML doesn't have reslocs

[quick shuttle]

@steady tiger I tried gradle refresh dependencies, gradle clean, gradle refresh. I am still not seeing the latest PR change. Does the bot not publish changes to the existing publish?

[steady tiger]

did you update the version?

[quick shuttle]

update the version to what?

[steady tiger]

https://github.com/neoforged/NeoForge/pull/1199#issuecomment-2197784051

126 is the latest commit

[quick shuttle]

124 -> 126 i didnt see that... I thought it was saying 124

my bad

[steady tiger]

you're bad

[quick shuttle]

no u

[steady tiger]

i need a tgpat emoji

[quick shuttle]

ok works now but I still see no log for translation key of config type files

idk what the format is to translate them

[quick shuttle]

sections do not read specified translation keys

[drowsy iris]

Urghsl, I suppose I should start porting AE2 back from its own config system to FML configs to test this, huh? 😄

We should pin the link to the PR

[quick shuttle]

Config PR: https://github.com/neoforged/NeoForge/pull/1199

[drowsy iris]

Thanks!

[last iris]

in reverse you're the builder methods calling

[quick shuttle]

what?

[last iris]

translationKey BEFORE push

[quick shuttle]

ah

[last iris]

same as with values

that's the one issue with this half-builder pattern. Without a .build() at the end but a define(name, params) you end up having to call the methods in an unintuitive order.

[drowsy iris]

So since this is "Config stuff" and not just "Config UI" 😄

First thought on porting:

I have to give it an IModConfigSpec.... ok.... have to attach sources first to figure out that I have to use a ModConfigSpec from Neo instead

Then the search for the constructor begins...

@severe sorrel I think we should make ModConfigSpec.builder() at least an additional method for getting the builder instead of searching through the inner classes to find it

[severe sorrel]

Yes

[quick shuttle]

is there a way to mark a config as needing a game restart?

I got some mod compat setup configs that requires game restart to take effect

[steady tiger]

see PR

[quick shuttle]

i dont see any example in config pr

[drowsy iris]

The builder doesn't make it clear if you can add comments to sections

And if so, whether you should comment, then push, or the other way around, or if it doesnt work at all

[severe sorrel]

I dislike the nested builder pattern

[quick shuttle]

tech's PR I guess. But it only has world restart. not game restart

[steady tiger]

comment push

push resets

not that PR TG, I have a pending PR for it

[quick shuttle]

o

[quick shuttle]

MERGE FASTER

[last iris]

at the moment, game restart is only triggered by Type.STARTUP

[quick shuttle]

So many translations to do.... i am physically dying

[drowsy iris]

@severe sorrel Turns out, we want this in AE2: https://github.com/neoforged/NeoForge/pull/1285 😄

[severe sorrel]

Unsurprisingly

[drowsy iris]

Oh, I actually was able to solve that one.

Next question: the PR description does not actually have an example on how to use this new config screen

Ah, container.registerExtensionPoint(IConfigScreenFactory.class, ConfigurationScreen::new);?

[steady tiger]

mhm

[drowsy iris]

Oh god..... I'l lhave to write a looooot of translations, ain't I? 😄

[last iris]

It's in the PR to the documentation, however that one is far from finished and outdated in some aspects

[drowsy iris]

We should just plonk that one line in the PR since some people are trying it out

May I also reiterate how much I like how some of the recent changes are playing together?

• Side-specific @Mod annotation for a Client-only entrypoint
• Dependency injection of ModContainer
• Registering configs + extension point for config screen on that ModContainer
  ❤️

I think we should make it more clear that you're about to be forced to restart the game when you change one of the STARTUP options...

Or just make Not yet... close the config UI

[last iris]

once the PR that keeps restart-relevant config changes from being visible from mods gets merged, that screen will change to a warning and allow you to continue. So this is temporary

atm the issue is that mods see the changes to those values, which for restartWorld keys could be fatal

[quick shuttle]

what if we add an option to dump a formatted en-us translation of all the mod's config entries and then modder can copy/paste that into their lang file and then translate each entry. Smoother migration

[drowsy iris]

Okay, do we not register STARTUP configs with the file watcher?

[last iris]

we already dump all untranslated keys. And I was contemplating pre-formatting that as copy&paste-ready json instead of a flat list

[steady tiger]

all configs are registered with the file watcher

[last iris]

^ which is part of the reason for that PR

[quick shuttle]

where? I see nothing in my console

[last iris]

check if the config setting (neo->client) is off. it defautls to on, but...

[last iris]

oh, and check if FMLLoader.isProduction() is false for you. I'm not sure what the logic behind that one is, I just took it from the other two "log in dev" things

[quick shuttle]

it is not printing

[last iris]

you need to exit the config screen. it collects all keys as it encounters them

[quick shuttle]

So you have to open all config screens, close out of it without quitting game, in order to get the logging?

Ok i see. I'm sorry but imo, I dont find that acceptable

it is much better to print on config screen open at the very least

[last iris]

yes. as I said, I'm not scanning the config as a whole and copieing it to a second tree structure like the old system did. lightweight

[quick shuttle]

otherwise, you lead to modders like me who have no idea how to trigger it

do the scanning in dev. Dont scan in prod

[drowsy iris]

I have to data-gen those translations, it'll be interesting trying to somehow marry the config-spec to a default english translation hm

[last iris]

that would be a completely new and independent piece of code. Also, it would need to duplicate much of the existing code to be able to duplicate not only its behaviour but also the subclassing mods may do with custom UI elements

it would have to recursively open all screens by simulating clicking on the section buttons and hoping it can detect all mod-supplied custom section buttons but not accidentally click on any mod-supplied "do something potentially destructive" buttons...

[quick shuttle]

45 more configs to do left....

[quick shuttle]

@last iris Was the ability to add to a list config forgotten?

I see no way to add a new list entry

[last iris]

you need to supply a Supplier for a new element to defineList()

the List inside the config element is a List<Object> and there's no additional type information. I decided against guessing what to stuff in there (I coded that first but it was ugly and error-prone, and for empty lists it didn't work at all).

[quick shuttle]

So I just return an empty string here?

[last iris]

yes

although for that list, I'd go with () -> "_bee" ;)

[quick shuttle]

No it must match file that people could name in any possible way

I do _bee. Others might not

[last iris]

in the example in the test mod I have a list of item resource locations as string, there I give it () -> "minecraft:". Just as hint...

in the end the important thing is the data type

oh, btw, lists with mixed data types technically also work in the config system. But I did not add support for that to the new-element supplier because it's just weird. But it can get even weirder, I found one mod that stores lists of lists and the inner lists have strings and int...

oh, and if its a filename, you can add a validator that checks if a file of that name exists. that also works in the UI

[quick shuttle]

341 lang entries done for my configs

god

[quick shuttle]

can a log warning be thrown if it detects a config entry using the deprecated method that explains the replacement and why a supplier of a default entry (Wasn't obvious the supplier was for providing a default)

[quick shuttle]

also, curious, how soon are we targeting for config gui release?

[severe sorrel]

as soon as it's ready IMO

[quick fable]

I think it might be worthwhile for at the tooltip on the buttons to show the filename (which is not localizable) anyway, in addition to any human-readable description that the button or tooltip displays

that'd solve the ambiguity problem

[steady tiger]

@severe sorrel so do you have ideas on how to check the restart types

[quick shuttle]

@last iris just verifying, you did see the stuff above right?

Also, the extra spacing here for categories is a bit weird.

ah nvm, need to remove my blank tooltip lang entry

Hmm is there a way to expose ranges?

right now you have to guess and trial and error to find max or min limits

maybe when you delete the number, it shows the range in italic gray in the box until you start typing a new number

I notice if you have a slider focused that has a large range, using the arrow keys makes the value jump up or down in larger increments when i expect it to only increase or decrease by 1. I would expect keyboard arrow keys for fine tuning the value

I cant get 93 for example in this slider of 0 to 255 with the keyboard. Only by dragging slider carefully with mouse

think that's it besides the untranslated warning that i would've liked to have at startup rather than entering-exiting menu but I wont block PR for that. Just that quick needs a big red note on the PR description and in dev announcement so people know how the warning works

As a compromise, what if you print the untranslated strings as you go through the Screens? (printed once and not again for that specific string of course if flipping back and forth between screens). That way people who see untranslated stuff on screen also sees the entries in console right away

[last iris]

it's in the tooltip...when using the comment as tooltip. I forgot to mimic that when using the language fie tooltip...
edit: done

[last iris]

the slider behaviour is 100% vanilla...

[last iris]

in the UI: no. The UI doesn't know if the deprecated method was used or if an intentional null (this list cannot be added to) was passed. In the defineList(): That kinda goes against marking them deprecated instead of removing them.

[last iris]

yes, what else did I not recognize as needing an answer?

[last iris]

and fixed the missing range.

BTW: PR is ready to merge at any time unless someone else wants something changed.
(Aside from the narrator...)

[last iris]

That would make it annoying to copy&paste the strings

[severe sorrel]

We need a new method in IConfigSpec

[severe sorrel]

One that only gets called once right when the config is registered

validateSpec or something

[last iris]

better bubble the values up from the values to the ModConfigSpec, query that in the register() and complain there. If there's even something to complain---the worst case I see is some value having a restart flag that is superseded by a weightier one, (e.g. for a value in a STARTUP spec needsServerRestart has no effect because the whole spec forces a game restart). However, for the value caching it needs a way to bubble down, but the values have access to their spec and can do "spec.needs || this.needs" easily. So setting this on the IConfigSpec upon registering would be needed. Alternatively, and I think this would make plenty of sense, moving the type from register() to builder() would handle all this nicely.

[steady tiger]

[last iris]

I'd even go so far as to say that moving the filename to the spec (register() -> builder()) would make sense, too. You can't reuse a spec for 2 files anyway

[steady tiger]

well the spec is just the specification of the file

[last iris]

not quite. the spec gets married to one specific file when it's registered. It isn't a stateless blueprint that can be used over and over...

from a database design perspective, it's a 1:1 relation, which would normalise the filename and type into the configSpec.

[quick shuttle]

No way to override the behavior for our stuff? Vanilla isn’t always perfect

[quick shuttle]

The strings can’t be copy pasted all at once anyway right now because they aren’t in lang format. They are just lang keys

By that, I mean if you printed them like
"mod.configuration.key": "",
Then you would have a point as then I can copy paste the entire block of lang entries all at once to the lang file

But just the key itself means I already have to do it one at a time so it makes no different if all are printed at once or printed as I see them in ui

[quick shuttle]

You can just make the deprecated method print right as used in startup? It doesn’t need to wait for ui stuff

[quick shuttle]

Random thought about us showing filename in tooltip for the buttons automatically

But I may just do it myself through tooltip lang entry. Leave it to modder choice I guess

[last iris]

thanks for the chuckle... ;)

You're out of date, I changed the printed format earlier as nobody voiced an objection when I suggested that

[last iris]

I'd rather drop the deprecated methods. The API change is trivial to adapt as it's just an additional nullable parameter

[last iris]

that has been suggested a couple of times, so I may just do it. It is rather trivial and won't hurt. I'll just make the "you can't because" part of the tooltip red

but it's annoying that I cannot get the actual path from anywhere, all I get is the filename part. As far as I see, the actual path is stored only in the filewatcher, and there It's buried deeeeeep.

[quick shuttle]

It’s ok, it can be something modders add manually to the tool tip if they want to

[quick shuttle]

That would make this config gui breaking. Question is if people can find the replacement easily. If this config gui doesn’t get finish before we go stable, then you’ll have no choice but to not make it breaking

[steady tiger]

FMLPaths.CONFIG.resolve?

[last iris]

it's more than getting the base path, the config loader does some logic and checking an override file and stuff. Too much that I would simple recreate the logic, even if it would boil down to something simple. Copying code that needs to be kept in sync without it fatally breaking when it desyncs is never good

[quick shuttle]

Serverconfig is not right tho. Don’t they load from config folder now as a default for world? What if you want to change that default?

[last iris]

No idea. the UI exclusively works on what's loaded into the spec.

[quick shuttle]

@mystic temple since the server config behavior change. It’s now server configs in main config folder as default right?

[last iris]

I can't even load another file the way the file is bolted to the spec. I'd need a way to clone the spec tree completely if I wanted to edit another file.

and the -server file is in configs. so yes, to get a per-world config, the user needs to manually copy the file to the world now

so editing will edit the installation-wide default unless the user has manually overridden it

btw, whoever placed that readme file there: Good job!

[quick shuttle]

The tooltip would ideally say this. “You’re editing the default server config settings. This will be copied into any new world you create afterwards”

[last iris]

yes, if I had a way to get that information, I'd certainly put it in there. Although the user should remember that they manually copied a file...

I'd suggest we get this PR merged, then work on the "changed values don't show up for the mod if they are marked "restart"" PR (I can take over this one once it's rebased), then isn't there a second one pending?, and then do another pass on fml side to expose that information and a proper name.

there's no way to ask the bot to watch a PR and post its github updates here, too, isn't there?

I'll just do it manually: ;)

(completely OT: In case anyone's wondering what my avatar picture is: That's the old conduit "in/out" arrow texture. I never liked the new textures we got post-1.7, but at that point crazy was still around and I hadn't yet taken over, so it wasn't something I had a say on. So I kept my mouth shut and silently protested this way. And now, dear painkillers, please kick in, my shoulder's killing me.)

[vivid surge]

I thought that isnt how it works anymore? defaults arent copied but instead loaded directly unless an override exists in that world

[last iris]

it is.

also, it would be nice if we could have a button "create override for this world". I'll keep that in mind for when we do some fml patches for this later

[vivid surge]

another nice button would be a 'backup file' and maybe even 'backup <all/all of mods/all of type> to zip' button

[last iris]

I'll keep that in mind. However, I think keeping the last x versions of any config file and offering a way to restore them and leaving the backup to whatever backs up the whole installation would be more useful.

nope, I won't keep it in mind. I added it here: https://github.com/neoforged/FancyModLoader/issues/183 (all the file handling is in fml)

[quick shuttle]

I think these two being merged first before config gui release would be beneficial tbh. So announcement can mention the world and game restart option and that people can use client/common configs for more events. Then people can refactor their config code as they work on the gui aspect

[quick shuttle]

What is special about the narrator stuff? is it hardcoded?

[last iris]

yes for the former (there's no overlap there), but I'd prefer the latter to after. Otherwise, I'd have to handle the merge conflicts and re-test the whole config UI again. If it comes after, I can concentrate on the changes needed in the confirmation screens in a fresh PR and have a clean diff view against a tested state.

[last iris]

yes, it needs some code to handle its sub-elements. The issue is that I have absolutely no idea how the narration system works and looking at code in similar vanilla classes didn't help me understand what is expected of me in that method.

[quick shuttle]

: thonk :

@steady tiger Thoughts on your PR going out after config gui? Tech and your PR is what I'll be waiting on before I release my mod with the config gui stuff

[steady tiger]

idrc about the order

[mystic temple]

Yes

[quick fable]

somewhat but vanilla has specific circumstances it uses sliders in - we already have an extended slider class, it'd probably be worth having an explicitly defined keyboard increment if there isn't one already

for the config case it's a particular concern since with the status quo being that text is the primary format i suspect a lot of mods use INT_MAX for the limit of something that doesn't have any obvious other place to set the limit

in general we should probably have a way to specify numeric fields as "yes it has a limit because we have to, but it's really intended that you pick a value that seems reasonable to you" and be a text box

a log scale slider - with 'clicks' at powers of two, powers of ten, or like 1-2-5-10-20-50, could be a useful thing to have, too.

[last iris]

That is why there are all the create methods. Mods can subclass and override those to customise all elements to their desire

[wise ember]

sooo ConfigOps for codec based configs when? /s

[steady tiger]

@visual bobcat https://github.com/neoforged/FancyModLoader/pull/184#issuecomment-2230979491 i feel like I'm going in a circle on github, it doesn't make sense for a server config to require a game restart when it cannot affect anything at startup..

[visual bobcat]

Please move this to github

The discussion is happening there because it is preserved and does not fall of the table.

[steady tiger]

there's this thing called synchronous conversation and async convo

and on github for the past 6 hours I've been going in a circle of "elaborate more" and "I see no reason for this"

[visual bobcat]

Yes and that is exactly why that is there

I and at least 1 other maintainer are not convinced that this is needed

And I want that discussion on the record in github

This thread dissappears

So when we are searching for this information in a year or 2 or more

We won't find it

But the PR will be there

And the commit that adds it will be there

So having that discussion on github in the PR, as to whether it is usefull or not

And why it is added

Needs to be there

[steady tiger]

discussion can happen here and be summarized on github

[visual bobcat]

?

No......

[steady tiger]

yes?...

[visual bobcat]

Why do you have an issue

The entire time you could have been just answering my question

Instead of discussing where to discuss it with me

[steady tiger]

I'm not answering your question or are you choosing to ignore it?

[visual bobcat]

You are choosing to not answer my question

?

Or what?

[steady tiger]

you've had me elaborate something I have already elaborated

I'm not going to repeat the same thing 3 times on github

[visual bobcat]

Where? Not in the PR, at least not that I can see

And the amount of elaboration does not seem to suffice, because I can not understand why you consider them different, when at least 2 maintainers consider them equal

[steady tiger]

why?
https://github.com/neoforged/FancyModLoader/pull/184#issuecomment-2229323018
could you elaborate more?

[visual bobcat]

?

Yes those are completely valid questions

[steady tiger]

?

I would really love to know why you consider them different contexts when I've already explained why they're not

one is a singleton, the other is not, a server config cannot affect startup behaviour

[visual bobcat]

You are in the FML repo, I now understand that this PR belongs to something in Forge that was not added to the referenced PR (which should have been in draft BTW because of that).

I then asked you to elaborate as to why there was no evidence in that PR for the requested FML change.

And now I am asking you to elaborate why you think that the two different contexts of the dedicated server and integrated server need to have the same behaviour.

[steady tiger]

a game and level restart are not the same thing

[visual bobcat]

Obviously they are not, but that has no bearing on whether you need to validate in what config they are used.

A server config can contain both config values that need a game restart or level restart

Just means that the user needs to do the same thing on a dedicated server

[severe sorrel]

Level restart only makes sense for server configs

[visual bobcat]

And a different thing on the single player integrated server

[steady tiger]

what, how can it?!

what server config value will require you to boot your client up again

[visual bobcat]

You have 0 idea what a modder might or might not do with that game restart config value.

[severe sorrel]

We are defining the API, we can set standards for how that value is used

Even then, I don't see how a server config could possibly affect game startup since it is only available once a server is started

[steady tiger]

except the config is loaded ONLY when a level is loaded and unload when the level is unloaded

[visual bobcat]

Look I see it simple as this:

The level reload flag already existed, so it is not new API or new behaviour, we never did something with it, but it was always there if I remember correctly. (I know coding a globe icon into the previous attempt for a config UI, so it already existed back then)

The game restart flag, is simple not relevant for any config other then STARTUP. And in reality any change to that config should simply trigger the restart.

So it is simply not needed to have the flag, nor do the validation

[severe sorrel]

The game restart flag can also be used for CLIENT and COMMON

[last iris]

No. It also could be a client config that does something to the rendering (e.g. HUD) that only happens when the ingame view is built. That may very well not take effect unless the world is restarted

[severe sorrel]

That is true

[quick shuttle]

To me, if I as a user see a config marked as Game Restart and another as World Restart in the server config on a dedicated server, they mean the same thing to me. Both require the server to be restarted so they are functionally the same.

From a modder perspective, it’s the same thing too.

Integrated server is where it is different where the server config might say to exit/re-enter world or restart game

[visual bobcat]

That too

[severe sorrel]

TG that is just not true in singleplayer?

There is a difference between re-join the world and restart your client

[visual bobcat]

So?

But not for the server config

[steady tiger]

as I said already

on a dedicated server

[visual bobcat]

For the server config they mean exactly the same thing

[steady tiger]

the config UI doesn't even exist for dedicated servers

[severe sorrel]

Yes hence it would make sense to disallow setting "requires world restart" for server configs

[visual bobcat]

No?

Why?

[last iris]

and if it did, it would upgrade any restart type to game restart. automatically and silently

[severe sorrel]

Because that's semantically what it means

[visual bobcat]

Both flags in the context of the server config mean semantically the exact same thing no?

[steady tiger]

you mean game restart, right? lol

[severe sorrel]

Yes

[quick shuttle]

Wouldn’t it be the other way? Disallow game restart and allow world restart? Since exiting and re-entering would load the new server config values. Ideally.

[severe sorrel]

Not if it needs to be interpreted by a client player

[steady tiger]

orion, you cannot argue that game restart and world restart mean the same thing for singleplayer

[visual bobcat]

In the context of the server config? Yes

[steady tiger]

?

[visual bobcat]

In the context of client and common, no

They are different

[quick shuttle]

So the worry is you don’t want players to restart the entire game if changing server configs in singleplayer

[visual bobcat]

But are allowed there anyway

[steady tiger]

you can restart a SP world without restarting the game?......

[severe sorrel]

Anyway I'm already getting annoyed. Please consider the value this pointless discussion is bringing

[steady tiger]

they're NOT the same thing

[severe sorrel]

(roughly ZERO)

[visual bobcat]

It is not pointless tech

[quick shuttle]

And by disallowing game restart, it would force modders to double check they aren’t accessing server configs before a world

[visual bobcat]

Except it is a runtime check

[drowsy iris]

What are options A and B that are even argued here? 🤔

[visual bobcat]

Not a compile time check

[severe sorrel]

It is entirely pointless. Don't interfere with people that are trying to move things forward

[drowsy iris]

Ok

[visual bobcat]

Don't do validation, and assume that a UI can properly display the stuff (lets not even talk about file changes).

Or introduce runtime restrictions on what kinds of config values can be added to the server config

[drowsy iris]

Not doing validation is the opposite of what you usually argue for

That seems weird

[last iris]

as I see it, this is about preventing people from dropping a grain of sand on their foot. An extra game restart after changing a config when a world restart would have been enough is far from anything impactful.

[visual bobcat]

Because it is what we have right now, and I personally think there are no concequences here.

[drowsy iris]

But if there are no consequences why argue this much about it 😅

[visual bobcat]

So it increases complexity that is not needed

[visual bobcat]

It increases complexity significantly that is not covered by a compile time check

Solely by a runtime check

[severe sorrel]

What? It decreases complexity by eliminating an error-prone state entirely

[drowsy iris]

Just to confirm: is this about validating that someone did not mark an option as requiring a full restart in their SERVER config?

[severe sorrel]

Yes

[visual bobcat]

Yes

[drowsy iris]

Ah, thank you

[visual bobcat]

But solely on the server ocnfig

[quick shuttle]

So my understanding is this is single player only.

You open up config UI and see 2 server configs. One marked as Game Restart and the other is World Restart.

If you change the first one, your entire game shuts down and re-opens. Then second one would boot you out of a world and you re-enter.

So I’m leaning towards maty/tech where we don’t really need to provide game restart to server configs as world restart should be good enough and not require waiting for pack to load again. And if dev abuse serverconfigs to need game restart, what the fuck are they doing lol

[visual bobcat]

So you now get three kinds of config values

In four kinds of configs at least

In which one of those combinations would be disallowed

[last iris]

or marked a value in their STARTUP config as needing a world restart. However, that would have absolutely no consequences

[visual bobcat]

Combinations that have no concequences should not be prevented in the first place

[drowsy iris]

I am getting the feeling our config system has some deeper issues w.r.t. its data model

But that is likely not something we can fix now

[last iris]

that is true

[severe sorrel]

"errors that are currently not causing errors should not be prevented"?

[visual bobcat]

It was never really supposed to hold this kind of metadata

[drowsy iris]

Introducing STARTUP might have been a mistake 😄

[visual bobcat]

It is not an error.......

The config value works perfectly fine

And does its job

[last iris]

STARTUP is fine. it is the type where this matters least.

[drowsy iris]

Well no, I meant conceptually

since now having a flag "requires restart"

[quick shuttle]

Startup isn’t involved here

[severe sorrel]

It is at best confusing

[quick shuttle]

This is just server configs

[severe sorrel]

It likely indicates an error in the user's understanding of server configs

[visual bobcat]

We solely have an issue about server configs.

[drowsy iris]

we do not have dedicated server configs, yes?

[visual bobcat]

No

They are considered the same

[last iris]

startup only comes up because there the game restart is enforced based on its Type anyway, no matter what is set on the individual config values

[quick shuttle]

Server configs are the ones we load from the world

[drowsy iris]

Which is what I was about to get at

[visual bobcat]

And the only reason server configs come up at all

[drowsy iris]

It's really WORLD configs, isn't it?

[steady tiger]

yes

[severe sorrel]

Yes but don't move the goalpost 😄

[visual bobcat]

No, they are actually bound to a server instance

That is why they are called Server

[drowsy iris]

Conceptually if you "load them from the world"

[visual bobcat]

They can also be loaded from defaultConfigs, or any other path the creator specified

They are just bound to the server instance

[drowsy iris]

yes, well, but lifecycle is boud to the world

[last iris]

server as in "the object that runs a bundle of levels", not "the software you install to run a server"

[visual bobcat]

And synced if the server instance is remote

[severe sorrel]

Their lifecycle is bound to a world

Obviously that is managed by the server

[visual bobcat]

No, again, their lifecycle is bound to an instance of the server

[severe sorrel]

But the data fundamentally belongs to the world

[visual bobcat]

Whenever that is destroyed or created is when the configs are unloaded and loaded

[severe sorrel]

Yes but conceptually this is world data

[visual bobcat]

The data does not belong to the world either.

That is simply the default path where it loads them from

You can put in any path

You can even make a server config, load from a central directory

[severe sorrel]

No no no, they get copied into the world folder once

Or I guess not anymore hmm

[visual bobcat]

They shouldn't be. In the past you could pass in your own path (factory)

Which should not copy stuff

But straight load it from the path returned

[last iris]

and yes, we do not have a config type for "hey, standalone server, here's the credentials to the database that one mod stores xy in". You wouldn't want to syc that to the clients

[severe sorrel]

You can't pass anything except a filename

[steady tiger]

there's only relative file names

[quick shuttle]

What about stranger file names

[severe sorrel]

Parent file names would like a word

[steady tiger]

just don't delete the child

[last iris]

current logic is "if there's a file in the save folder, use that. otherwise use the one in the config folder". Just in case that wasn't clear.

and yes, "../../../../..windows/system32/cmd.exe" DOES work as a config filename (which is scary)

[quick shuttle]

Anyway, I don’t see a need to block Mary’s pr. Seems fine to enforce modders to not require entire game restarting when they only need world restart in single player

[visual bobcat]

IT does

[steady tiger]

who's getting married

[steady tiger]

we really ought to fix it to relative only with no walking upwards lol

walking upwards makes no sense when you load it from ie defaultconfigs or the world config folder

[severe sorrel]

I mean if you're running arbitrary code (i.e. mods) you can already do worse things 😛

[steady tiger]

(do we still have default configs, or just the world config folder for those, I don't remember)

[visual bobcat]

Okey I think I can approve this,

Question how are these flags going to work with changes from disk?

[steady tiger]

well, you can't exactly resolve that path in the world config folder can you 😛

[last iris]

yes, but malware scanners can detect a file open call. they cannot detect if you build a scary string for the cnfig fle

[severe sorrel]

That comes in via acceptConfig

[visual bobcat]

Sure but functionality, do we popup a UI or something that indicates a restart is required?

Or do we dump something in the log?

[steady tiger]

I mean it's not even about malware, it's about actually resolving the paths from different locations

[visual bobcat]

Or what is the underlying idea?

[last iris]

try out the UI PR (which is ready, btw). it can handle those cases already

[quick shuttle]

But does config ui do restart automatically or is it just a warning to user

[last iris]

there's another PR that stops values loaded by the file watcher from being visible to the mod if they are marked restart-needed

[steady tiger]

without the neo PR it forces you to restart

[visual bobcat]

Can you explain this to me real quick, in like a sentance or two. Also how does this work on the dedicated server then if it is part of the ui changes?

[steady tiger]

as mods will have the values updated with the flag ignored which can be trouble

[last iris]

it does. but that will be downgraded to "restart/ignore" when that other PR goes in

[quick shuttle]

There’s no ui on dedicated server. You edit configs files directly on dedicated server

[visual bobcat]

Yes

But what does it then do?
When one edits such a config entry

[last iris]

Orion: When you exit a mod's config, it will check if there are changes in values that are tagged needsWord/GameRestart. If so, it will display a screen that has a "exit game"/"resturn to main menu" button and at the moment a "back to config" one. Once the other PR is trough the latter will be downgraded to "back to game (I know changed values won't take effect"

[visual bobcat]

I completly understand that...... I wrote a config UI once.

What I am asking is what happens when the file gets changed and reloaded!

[last iris]

at the moment, the server will simply pull in the changed config. that's what that second PR will fix

[visual bobcat]

Not what happens when you edit it via the UI

[visual bobcat]

Does the client not also just pull in the changes?

[last iris]

yes, without that PR the needsRestart(9 flag is 100% ignored. It was put in back then for the UI but never had any code behind it

[quick shuttle]

Wonder if we could make it so marking a config as world restart and such will add an extra comment to the config file saying what restart is required. So editing file directly lets you know what restart to do

[last iris]

Oh, I have an idea. rename it from needsWorldRestart() to needsRestart() and let the config type decide if this means world or game.

[visual bobcat]

That seems much more reasonable

But then the config validation does not seem needed

[steady tiger]

didn't you just suggest world restart for client configs lol

that's not going to work

[last iris]

naturally it's server->world, else->game. That's the only safe thing to do...

[visual bobcat]

Yeah

But then no validation is needed

[last iris]

although, startup->game else->world also would work. It just means modders need to be careful nd use startup...

[visual bobcat]

Depends

[steady tiger]

bbbbut, #1134858821160931471 message

your own opinions are contradicting

[quick shuttle]

People can change stances

[last iris]

aaanyway, I think we're overcomplicating things. the worst thing that can happen if we do nothing is that modders set weird restart requirements on config values that either do nothing or force a game restart when a world restart could have been enough.

[visual bobcat]

That is kind of my point

But if people feel like preventing this

Then that is fine

[steady tiger]

especially if client configs are loaded earlier, a client config could be for the GUI and another one for idk, renderers

[last iris]

we're also not preventing people from writing onUse() methods for blocks that don't have a hitbox, so what...

[steady tiger]

that's not apples to apples, one is a mojang thing and the other a feature we provide 😛

[last iris]

then : SubscribeEvent on a method with an empty body

[steady tiger]

that's not feasible, but you know what is? preventing people from registering mod bus event listeners to the game bus and we do :P

enum extensions are also full of validation, if you don't validate a specific thing that's not an argument against validation in general

[last iris]

still would more sense to me if the register() would figure out which bus to use instead of a complaining...

[drowsy iris]

For next break cycle I'd already like to apply for removal of STARTUP and moving up of CLIENT and COMMON to where STARTUP was 😛

[lilac leaf]

Before registries?

[quick shuttle]

the entire point of startup is to load right at registration of the config. So it can be used for anything that needs early configuration. And it has javadoc for explaining the pitfalls for using it for registration of server/client synced stuff.

Moving client and common to load at config creation will lead to far more people abusing it to do server/client synced stuff as they won't look at the javadocs

I honestly thinking about putting a deprecate tag on STARTUP only to get people to read the javadoc that would say it's not actually deprecated but here are pitfalls to beware of.

I'll tell you right now, Curle will shoot you if you move client and common to before registration lol

I probably might too

[drowsy iris]

It is hella inconsistent

[last iris]

I'd rather suggest to replace startup as Type with registerPreload(). That way mods can have client and common startups if they want, and there is a nice method to put a mile of javadoc on. Preloading would, just as startup, load the config when its registered, but then treat it the same as any other config, i.e. put it into the same queue to be loaded later, firing the same configLoaded event when that happens (and maybe even not fire one when it's preloaded).
I'd also like to suggest renaming SERVER to SHARED to make space to later add a real server (integrated and standalone) config.

[drowsy iris]

And bad UX overall

I'd rather rename SERVER to WORLD

If we still support storing the config-file in the world-save directory

Which I think we do

[last iris]

I can go with WORLD. didn't want to suggest it as it's controversial, see above

[drowsy iris]

but grunt regarding STARTUP: it's confusing as hell

I now have a single (read: 1) configuration option in a STARTUP file

because I can only affect hotkey registration before the actual config it should be in (CLIENT) is loaded

that is bad ux

and bad devex

[last iris]

or, and I'm thinking in extremes again, we could change Type into a bitfield.. (client, integrated server, standalone server, preload, sync on join, load at game start, (re)load from world folder)

[quick shuttle]

double check if tech's PR will make it early enough

he is moving it to right after deferredregisteries

[drowsy iris]

maybe it will, but generally it's just very arbitrary when it happens from a user-perspective of the API

[quick shuttle]

You wont be able to get common and client configs before deferred registries without meeting a ton of maintainer opposition

[drowsy iris]

Hence I already announced the intention for a formal petition! 😄

This feels like a "don't mention the war" situation

[steady tiger]

it is war

[pale venture]

isn't it sufficient to load those configs before the first resource reload to solve this

[drowsy iris]

maybe, but whatever we do now sucks

Techs PR might solve it

and I might forget about my complaints 😄

[severe sorrel]

It's merged so you can go and check 😄

[last iris]

sorry grunt, I had not noticed you posted 2 doc PRs in one message and only looked at the one for the 1.21 changes earlier. Otherwise I'd have commented earlier.

[quick shuttle]

i dont understand your comment here
https://github.com/neoforged/Documentation/pull/137#issuecomment-2240039670

this is for the doc website. this is not the neo code so i cant point to the javadoc in neo code.

[last iris]

I meant to write something like "If you want to extend that thing, read the effing javadocs of the class instead of asking for a YouTube video, there's a whole wall of text on that in there, written to tell you how to subclass." Just about 3 levels more polite. Or 4.

[quick shuttle]

I think they will read the javadoc when they go to extend it as they have to read the class anyway for what to override

[quick shuttle]

@last iris some people elsewhere (who refuse to come into this server or make issue reports), had a suggestion for the config screen. That is, to have a column of buttons on left side of screen that is every config file. So left side would have “Client” button and “Common 1” and “Common 2” config file buttons. When clicked will open the screen to show that file’s config option. The left side column of buttons will always show so you can switch between the configs easier.

[steady tiger]

(I would ignore all aggressive feedback honestly)

[last iris]

That's a nice idea for a config UI, but it'd be a completely different architecture to what we have. for one, we'd need to implement a custom base screen instead of using vanilla's, and then it relies on the screen space being bigger than the vanilla default minimum.

[quick shuttle]

That wasn’t that one person’s suggestion technically. It was the other person who was nicer

[last iris]

as I said, the two three major design rules behind this UI are (a) close to vanilla in code and design and (b) optional and extendible, can serve as an inspiration for custom screens, (c) minimal viable solution that doesn't take away viability from people who want to offer nicer, kitschier, more capable or colourful ones (Nobody expects the Spanish Editition)

[quick shuttle]

@tardy swift just fyi for here if you have config screen suggestions

[last iris]

so yes, we could overload this with all kinds of bells and whistles. But that's not the idea behind it. This intentionally is the "no bells and whistles" version, just like the ToastEvent isn't a config setting to configure which toasts are suppressed but something a mod can build on.

BTW: Can we fasttrack that Config-UI-Fix PR a bit? It is a slight change and adds 2 new things modders can do for their config, so it'd be nice to get it out before the majority of mods has migrated to the config UI and has built their own workarounds for the bugs and button positioning.

[jovial bane]

where PR

[last iris]

https://github.com/neoforged/NeoForge/pull/1361

[jovial bane]

why is there an HTML entity in a {@snippet} tag

[last iris]

where? got it

it works that way and as literal the @ is interpreted and not taken as a literal. I got that by checking the jep for the literal tag

[jovial bane]

left comments, nothing major though I do have a question in there

snippet should take care of nested @s, I believe
only thing snippet has trouble with I think is multi-line comments (specifically, the end of multi-line comments) in inline snippets

[last iris]

implemented one, commented on the other two

[tardy swift]

Was about to say, I make issue reports and am in here >:T

[tardy swift]

I don’t know if I agree with this characterization. I guess it might be more complicated than what you have but building an intuitive UI shouldn’t be treated as “bells and whistles” imo. The suggestion to put categories on the left or to integrate them into 1 full screen UI with sections would be better UX than what exists now. Isn’t the goal of the config UI screen to provide a better default UX for users than what config libs provide with having to click a button then scroll through a list of all the mod config libs to then find the one they clicked in the first place?

[last iris]

For everyone answering questions about the Config UI: Keep in mind that unlike most of Neo's systems, the UI is completely optional to use or roll one's own. This is not like e.g. item registration, where you must use the registration event and cannot implement your own way of pumping them into the registry. People may not expect that, and so be in the mindset that the UI must be able to do everything they need. ;)

[tardy swift]

I’m not expecting the UI to do everything I need, if I need more I’ll build on it. But this isn’t asking you to add new features, it’s asking you to improve the UX of the few features you do have

[last iris]

The goal is to have a default so mods that don't need something complex can opt-in with one line of code. Another goal is to stay as close as possible to the vanilla options screen look&feel (that users should be familiar with). Originally it didn't even have that "label-field" layout but used unlabelled fields in 2 columns like vanilla. I only removed that mode last minute as nobody seemed to prefer it...

But aside from the design goals; I gave the reasons for not considering this above: The current architecture doesn't support this, so I would have to throw what is there at the moment away and recreate it from scratch. Also, horizontal screen space already is at a premium with many config options' text being truncated to fit. (this may not be obvious in the demo screenshots as I have very short texts on the dummy options in the test mod.)

[tardy swift]

You could also use tabs, tho that’s not as nice

All the options laid out in categories would also work, and would be consistent with how vanilla does their settings like in the UI for key binds

[last iris]

I don't understand that one. What categories (subsections) there are is purely defined by the mod...

2 deep ^

[tardy swift]

? im saying instead of nested options they're just laid out flat. but that might not work out as well if you have an infinite amount of nesting

[zenith crypt]

Hello, I don't know if this has been brought up in some capacity already, but when testing the new (and generally awesome!) config screen with a recently ported mod, I noticed two things:

• One string config entry of our mod has a really long default value, which doesn't seem to fit in the config screen's edit box for the config entry and gets cut off. So when a user clicks into the box (without modifying anything else) and closes the config screen, the cut-off value is saved to the config file, which seems unintentional (this also means that the user cannot manually enter a longer string, which is almost required for that specific config entry).
• Another config entry we have is supposed to represent a color (as an integer in the 0xRRGGBB format), so it would be really awesome if, in the edit box for the integer, one could enter a number in that hexadecimal format and it would be parsed automatically (the same is currently possible by editing the raw config file and putting a hexadecimal number as the int value for the config entry, it gets parsed automatically).

[last iris]

For the first one: As a workaround you can filter that value out and replace it with a static label redirecting the user to the config file. I'll have a look at the limitations of mc's text box to see if we can change them or at least prevent too long strings from being edited. (if you want to add a workaround, I mean. I'll fix the issue one way or another anyway.) Edit: maxLength is 32 and is configurable. That's very low, I'd say, but on the other hand, there's not much space on the screen. Would 128 be enough for you? I'll also put code in to set the maxLength to the current length if it's higher, but I'll have to limit that, too. In that case there will be a notice.
About the second one: Copy&Paste should work with hex numbers; I remember doing something to allow those to be entered...but that was very early in the process, and I never checked if it worked or what else is needed to make it work. I'll have a look and report back on the current state, as well as put it onto my "to fix" list. (edit: I forgot checking if cop&paste works before fixing it. sorry)

[zenith crypt]

Our default value for the string config entry is 87 characters long, so I believe for our application a max length of 128 would be well enough, yeah; thanks for looking into this!

[quick shuttle]

256 would be ideal string max length. Any larger and modder should look into lists or something

Hmm what if a modder has a string for a url or path to a file

Those could get very long

[last iris]

I decided to use Mth.clamp(source.get().length() + 5, 128, 192) and display a label with a message to edit the file directly if it's over.

the EditBox is not at all optimised for long strings, it could run into issues with the font if this gets too long

[last iris]

PR1366. I'm leaving it in draft for a day or so in case more reports come in

[trail matrix]

Is there any reason why the startup configs require restarting the game? I use this, however the values do not require restarting the game.

I can't use other types of configs because the loading is much later than I need it to be.

[steady tiger]

they shouldn't?

[quick shuttle]

Are you sure you’re not using startup configs for stuff that only loads once

[trail matrix]

It seems to me a rather silly solution to lump all values into one comb just because it's a startup

[quick shuttle]

???

[last iris]

Startup configs are used while the game starts up for something that cannot be done later. Obviously, that use cannot retroactively be changed when the value changes, so the game has to be restarted. And if that something was something that could be changed later with a config reload event, you wouldn't need a startup config.

[trail matrix]

I can't move some values from the startup config because it's an xplat mod that uses a different library on the fabric-side

Also, I can't imagine how it would work if the same class called two configs at once in the same place

[last iris]

I have no idea what that is supposed to mean, sorry.
However, if you don't want the code that's using the config having to check if it's already loaded or not, you can wrap your config values like this:

    static ModConfigSpec SPEC;

    record Wrap<T>(ModConfigSpec.ConfigValue<T> cfg) implements Supplier<T> {

        static <T> Supplier<T> wrap(final ModConfigSpec.ConfigValue<T> cfg) {
            return new Wrap<>(cfg);
        }

        @Override
        public T get() {
            return SPEC.isLoaded() ? cfg.get() : cfg.getDefault();
        }
    }

[trail matrix]

Getting the default value if the config is not loaded is a bad option for me

[last iris]

if your config truly supports its value being changed at runtime, that won't be an issue.

[trail matrix]

You could have just said that hack forge would have been a better solution

Don't get me wrong, I want to use startup configs, I have used configured before and it didn't require a restart, however now you are suggesting I go back to basics and just load a different type of configuration before with hacks?

[last iris]

"startup" means "I need this value when the game starts and cannot set/change it later when the configs are (re)loaded". If that is not your reason for using startup, you're not using it for what it's there for. I already gave you the solution for "that value can be changed at any time, but there's code that also runs too early that will try accessing it".

[trail matrix]

"code that also runs too early" also requires detailed configuration, so I can't use default values for it

[lilac leaf]

What is your specific use case?

[trail matrix]

My mod tries to get config values every time the loading overlay appears, this applies to both loading and reloading

[pale venture]

the early loading screen or the regular resource reload overlay?

[trail matrix]

Both

Oh

Only resource

[lilac leaf]

With a recent change, you no longer need startup configs or hacks for that. Regular client/common configs will work for that.

[trail matrix]

What change?

[lilac leaf]

https://github.com/neoforged/NeoForge/pull/1285

[trail matrix]

No, that didn't work, at least replacing ModConfig.Type.STARTUP with ModConfig.Type.CLIENT resulted in the config not loaded when trying to call it

Now I have two options: either I load all previously created configs, or mixin to the gui (it's really awful, have you even seen its code?)

[lilac leaf]

Are you using the latest NeoForge build?

[trail matrix]

Yeah, it's 21.0.117-beta

[lilac leaf]

Are you accessing the config from a reload listener? If so, then it should work, and if it doesn't, that may be a bug.

[trail matrix]

The config value is called at the very beginning of resources loading

[lilac leaf]

Where is that exactly? Are you using a mixin?

[trail matrix]

at MC-BOOTSTRAP/com.google.common@32.1.2-jre/com.google.common.base.Preconditions.checkState(Preconditions.java:512)
at TRANSFORMER/neoforge@21.0.117-beta/net.neoforged.neoforge.common.ModConfigSpec$ConfigValue.get(ModConfigSpec.java:1171)
at TRANSFORMER/rrls@5.0.5+mc1.21-forge/org.redlance.dima_dencep.mods.rrls.neoforge.ConfigExpectPlatformImpl.hideType(ConfigExpectPlatformImpl.java:118)
at TRANSFORMER/generated_35ee3e3@1/org.redlance.dima_dencep.mods.rrls.ConfigExpectPlatform.hideType(ConfigExpectPlatform.java)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance$1.wrapOperation$zzi000$rrls$async(SimpleReloadInstance.java:548)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance$1.wait(SimpleReloadInstance.java:54)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.ResourceManagerReloadListener.reload(ResourceManagerReloadListener.java:12)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance.method_18368(SimpleReloadInstance.java:32)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance.<init>(SimpleReloadInstance.java:44)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance.of(SimpleReloadInstance.java:32)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.SimpleReloadInstance.create(SimpleReloadInstance.java:101)
at TRANSFORMER/minecraft@1.21/net.minecraft.server.packs.resources.ReloadableResourceManager.createReload(ReloadableResourceManager.java:44)
at TRANSFORMER/minecraft@1.21/net.minecraft.client.Minecraft.<init>(Minecraft.java:618)

And yes, it's a mixin

[lilac leaf]

That should work

I'll investigate a bit more slightly later

[steady tiger]

what okay, I'll rephrase. startup configs SHOULD NOT require a restart

[drowsy iris]

??? Now you are confusing me

[visual bobcat]

Yeah

of course they all require a restart

Their whole point of existence is that they can influence the registered objects and registries

Hence to be safe it needs a restart all the time

[steady tiger]

uhm, no, there's nothing preventing a startup config value from being changed lol, that's the point of the flags pr (which I'm yet to rebase)

[trail matrix]

In the value builder there is a method worldRestart, what was the problem to make something like gameRestart?

I remember seeing a PR that does this...

https://github.com/neoforged/NeoForge/pull/1284

[severe sorrel]

They don't need to require a restart IMO

Maybe the config enables a specific log level that can be changed at any time, idk

[steady tiger]

https://github.com/neoforged/NeoForge/pull/1284 pr has been (finally) updated

[quick shuttle]

The part about the config GUIs needing to use another method should be a dev announcement btw

[last iris]

PR for PR1284 is ready. But this is a breaking change as the UI needs access to the uncached values and cannot treat the config value as a mere Supplier.

[severe sorrel]

https://github.com/neoforged/FancyModLoader/pull/185

an easy one 😉

[last iris]

https://github.com/neoforged/NeoForge/pull/1284#issuecomment-2254115511

that would have been caught by the compiler with the RawProvider, um, no, even better, it would have worked automatically. That's one reason I used it, it would stop using the wrong getter by accident. Now we have to just be careful...

[quick shuttle]

I am not sure what "need the same treatment" means here

[last iris]

they also need to be wrapped to call getRaw instead of get

[quick shuttle]

so just

[last iris]

(I thought that change to have two sets in that switch was in the second fixes PR, but it already was in the first. That's why I didn't check if the PR³ edited both. my bad)

[last iris]

I don't think that compiles, that was the very first thing I tried. () -> value.getRaw()

[quick shuttle]

this is in my ide. It compiles

[last iris]

interesting. then that's fine

[quick shuttle]

ok it doesnt actual compile but it doesnt show errors beforehand

is my intellij just borked

[last iris]

lol

but I know that effect, I have it in Eclipse, too. But only with neo, not once had I had that before and I've always used Eclipse for modding and used at work since its inception

[quick shuttle]

working on the enum one but like, intellij keeps telling me I can just do method ref

[last iris]

try (Supplier) value::getRaw, that is how it was done in the first block for enums

but please test that one (neo config has enums, so it's an easy runClient and go to neo's config), as that one may be a runtime error...

oh darn, I shouldn't check notifications and discord first thing after gettng up. my brain still feels like it's packed in wool. grrr, I know I need an hour or so in the morning to spool up

[last iris]

:: for the cast, not .()

[steady tiger]

value::getRaw not value.getRaw()

you want a method ref

[quick shuttle]

maty

method ref doesnt compile

i tried that lol

[steady tiger]

a direct method ref doesn't compile

a raw one should and will

[last iris]

like in case ValueSpec spec when cv.getClass() == ConfigValue.class && spec.getDefault() instanceof Enum<?> -> createEnumValue(key, valueSpec, (Supplier) cv::getRaw, (Consumer) cv::set);

but, as I said, I'm not sure it works at runtime. I had assumed tec had tested it, but after the PR³ didn't even compile, I'm not so sure anymore

[quick shuttle]

will eh?

[steady tiger]

@severe sorrel test your damn code smh

[steady tiger]

that's not a raw method ref lol

(Supplier) value::getRaw

the cast is important

[quick shuttle]

fddfsdsa

[last iris]

ok, here's something I should have added as comment:
The calls in line 602+ are for the cases where a value is defined without constraints. We get a raw ConfigValue
The calls in line 610+ are for cases where there are constraints, we gat a subclass of ConfigValue
we need both because the 610s are "we know what class the value is" while 602 are "we guess based on the default" (another thing for my ModConfigSpec cleanup in 1.21.1)

[quick shuttle]

let's just go to c++.

[jovial bane]

alright, time to ship Tele to MC Bedrock

[last iris]

^ I know those go through line 614, so it's good

darn. I just noticed another minor bug. Comments get a "possible values: x, y, z" added for enums, that's missing when using tooltips. I will need to make another bugfix PR, but at least that will be a minor text tweak only. I hope. (Later, not now!)

and it's just for consistency, the cycle button already cycles through all possible values anyway. Unlike the toml file ;)

[quick shuttle]

Config issue report came in https://github.com/neoforged/NeoForge/issues/1385

[last iris]

on it, thx for the heads up

but isn't it just typical that a new report comes in just hours after merging the last fix, even though we let it sit there for days...

[jovial bane]

[last iris]

at least the fix was trivial. use the key instead of the key for the key.

[jovial bane]

blinks

btw, did ya'll check the accessibility of the config screen, just in case

[quick shuttle]

Need someone with narrator experience to do that part. No one knows so it didnt make sense to hold up the entire config pr as we might not find someone that can get narrator working

[last iris]

I checked keyboard navigation and otherwise relied on using native mc controls for that. There's one TODO item, that is the narrator for the widget that serves as list element control

"TODO item" as in "there's a big TODO comment on it and I asked for help with it in the original PR"

[jovial bane]

good good, as long as we know what's working and what's missing

i wonder if I should formalize some sort of policy that every TODO gets a corresponding issue...

[quick shuttle]

Which is why I volunteer sci to do narrator work

[jovial bane]

pls no(t yet)

[quick shuttle]

Opening the game to maximum volume “NARRATOR ON”

[last iris]

on the other hand, as there's no text but only symbols and numbers (and those are always in sequence), it should work for people who can't read or have other issues reading. Won't work the the blind, but I don't know how many of those can play the game

[last iris]

Good idea. https://github.com/neoforged/NeoForge/issues/1387

[severe sorrel]

Intellij was happy with it so it must have been correct

[steady tiger]

so naive

[quick shuttle]

With maty’s game restart pr, maybe the restart warning screen shouldn’t be forced with startup configs. If there’s usecase for needing startup but not restart

[last iris]

up til now nobody has managed to explain that use case to me. But to enable it, that screen no longer is a hard stop but an ignorable warning

[drowsy iris]

That is good btw

[last iris]

see my longer reply next door

[drowsy iris]

the ignore button

I still think we should make the case that we do not need a separate startup config-type

either by moving up the other config types, or by declaring "there is no such use-case!" (which I find.... flimsy)

[last iris]

I also think loading all configs on register and delaying the load event until after the registries are up would be perfectly fine

because the load event may need registry access (the MDK's does)

[drowsy iris]

that might be a compromise

is anyone on staff still opposed to just loading configs as soon as they're being registered?

[stoic cliff]

I think moving client and common configs to "as soon as registered" makes sense, but obviously that's a big break so 1.21.1+

[last iris]

I can't recall anyone being violently opposed to that when it was brought up before. Uneasy with it, but not horrified

[drowsy iris]

It just removes an inconsistency in my opinion

[last iris]

the only break would be the removal of the STARTUP type; I don't think anyone relies on a client/common config throwing an exception in a way that their mod would break

[drowsy iris]

under the condition that the event triggers later

[last iris]

yes, that' a given. firing the event at real load time before the registries would break half the mods...including the MDK

[stoic cliff]

I don't like the idea of firing the event at an arbitrary point that isn't when the config loads

[last iris]

think of it being a "ConfigsCanSafelyBeProcessedEvent" instead of LoadEvent.

or a "Please(Post)ProcessYourConfigsNowEvent"/"PleaseLoadYourConfigValuesFromTheConfigObjectsNowEvent"

[stoic cliff]

yes but it means there's a period of time during which the config values may be out of sync from what the mod thinks the config values are

[last iris]

we have that now too. It currently lasts from classload to the event

[stoic cliff]

like I don't think it should cause any issue, it just doesn't seem nice to me

[last iris]

if a mod copies a config value to their own field, it will be out of sync until the event (no change here). if not, it will have its true value with the change always instead of throwing an exception when accessed before the event

also, copying a config value only makes sense if they need to transform it, which often needs a registry (e.g. string-RL to Item/Block). So that can't be done any earlier, but it also should not be needed for many values.

we should discourage coying config values for no reason. that just prevents the player from making changes at runtime in the UI if done badly. (and I expect "done badly" to be the default as people aren't really aware of the lifecycle here)

btw, in 1.21.1 would could rename the Load/ReLoad events to PostProcess and ReProcess to further discourage people from thinking they need to copy their config values for some reason or another.

[quick shuttle]

@wet hound was hard against because she remembers when modders used to load configs during registration and have configs control whether to register something or not. Which then caused clients to break when connecting to a server that changed that config value due to mismatched registries. Forcing people to disconnect, ask server owner what the heck they changed, then change it on client and reconnect. Startup is an opt-in, javadoc heavy alternative for niche cases where config value is need early but not for controlling what is registered. The javadoc for is heavily stated this

Making common be loaded for registration will bring back this issue

[drowsy iris]

Have people who relied on this not already worked around this

AE2 moved to its own config system

[last iris]

people who want to conditionally register will do it anyway

[drowsy iris]

others figured out how to just use ModConfig but handle the lifecycle themselvs

[wet hound]

and always know when that will cause the mod to cause problems. the point is the people that don't know better. it only takes 1 badly written mod in a large modpack to really fuck up the user experience

that's why we currently have an opt-in (startup)

having early config by default is a no-go

like, just, no

[last iris]

how can you accidentially conditionally register?

[wet hound]

it's less about accidentally doing it, and more about not understanding how that causes users to interact with it

and how it relates to servers using the mod with different configs not being compatible any more

[drowsy iris]

soft incompatibilities like that still exist even with late-loaded configs

not hard incompatibilities though I suppose

[wet hound]

it's all about preventing the need for the user to restart between connecting to different servers

that's a serious hell that Neo will never devolve to

[last iris]

this has been that way from the beginning of time until 1.13. and happened so rarely that I can't even remember a case of seeing this

[visual bobcat]

I saw this happen in like every second modpack

Unless you player a large well maintained fully configured and tested pack

There was at least one mod to screwed it up

[drowsy iris]

It was a core feature of AE2 ™️

[visual bobcat]

😛

[drowsy iris]

Being able to turn every feature off

completely.

but since modpacks generally distribute those configs, I also never saw an actual bug report about this

[last iris]

what I did see quite often was mismatch in installed mods, not some player changing the configs

[drowsy iris]

To some degree this is still the case

AE2 still uses COMMON to define energy capacity values used in Items

[quick shuttle]

I think it would be ideal having startup be marked deprecated and then adjust javadoc to say it’s only deprecated so people can read its javadoc for the registration warning lol

[wet hound]

shartte seemed to want to remove startup entirely and move all configs to load before registration

[quick shuttle]

Correct and I am against that too

[wet hound]

(which will never happen. the steering council exists to prevent these kinds of easy footgun problems from being included)

opt-in or bust

[quick shuttle]

Startup was the compromise we did exactly for this

[drowsy iris]

yes 😄

I might actually retract that

[wet hound]

if you think we should go further and have it strikethrough in IDE, that's cool too

[drowsy iris]

I currently have a single STARTUP config

It's possible I no longer need it after Tech moved stuff around in one of the last PRs

it was for conditional registration....

.... of hotkeys 😄

[wet hound]

yeah, there are reasonable uses for conditional registration

armor properties, etc

[quick shuttle]

But keybinds are configurable in game already

[wet hound]

like, it's something you can easily find a need for

which is why there's an opt-in feature for it

[drowsy iris]

It's for disabling the "hold G to show guide" functionality in tooltips in its entirety which also disables the hotkey registration for it

[last iris]

in an ideal world, we'd only have two classes of config values, gameplay-relevant and cosmetic, and would load the former from the server and restart the game if there was a mismatch. Think Factorio connecting to a server with different mods, just for configs instead

[wet hound]

in an ideal world where modded minecraft loads as fast as Factorio, that'd be cool

[quick shuttle]

A game restart in factorial is what? 20 seconds? In large minecraft modpacks, it can be 10 minutes

[drowsy iris]

As long as we group config values in screens by their config type

I think what we have right now is a bit of a mess

That is the primary reason I am somewhat angry at STARTUP

[quick shuttle]

I think what we have is fine and an improvement over past. Give it time to settle in before throwing out the baby with the bathwater

[drowsy iris]

(And yes I recognize that I might not even need it anymore after the recent changes)

[last iris]

it'd be a greater mess if we were to merge configs together as that destroys any kind of organisation the modder might have put in

[drowsy iris]

The organization is purely technical

and not by feature

[jovial bane]

factorial 😭

[drowsy iris]

i.e. in my own config system that "disable guide hotkey" config was grouped in with all the other tooltip options

when moving back to the NF config system I had to split that out into a separate startup config

making it a complete mess in the config-ui (and file)

[last iris]

no. which config values to put into which sections and if sections can have the same name in different configs is not technical, it's a design choice

[drowsy iris]

No it is technical

if the feature is "X", whether you technically need a config value at startup or in common is a technical distinction

that is not necessarily related to the feature

but the grouping in the UI is by config-type first, then by section

[last iris]

no, how modder group or not group inside their configs is pure choice. merging things together that are not together in both the code and config file would be a mess

[quick shuttle]

^

[drowsy iris]

So imagine AE2: there's three config buttons "COMMON, CLIENT, STARTUP"

where STARTUP shows you a glorious single option

that is bad UX

[quick shuttle]

Users would then know startup is a special config probably for changing the state of the game that it launches with and likely needs a restart

[last iris]

for something I still don't understand the reason for. not registering a keybind and registering it with no default key make no difference

[drowsy iris]

we have "needs game restart" for other options too grunt

not just for STARTUP

[quick shuttle]

Look shartte, as much as you want to mash everything together, purely for what visual look you prefer, the footguns with loading the other configs too early is too great. The problems it will cause far outweighs the “terrible” sight of one extra config section

[last iris]

but now imagine a mod having 3 "show X in HUD" options in CLIENT and 3 "may helth boost of mob" options in common. now we mix them together in a random order and show them on one screen. That's even worse

[drowsy iris]

presenting config options in a sensible grouping is not purely visual

why do you claim they would be in random order?

we do have sections, do we not?

[last iris]

seamingly random order

[drowsy iris]

Sorry but you have enough precedent in thousands of other games

[last iris]

BUT WE DON'T FORCE MODS TO PUT STUFF INTO SECTIONS. that would be a major breaking change

[drowsy iris]

I really don't have to make a case here

Just look for prior art

I cannot remember a single game that grouped its configuration options by "this requires a restart" and "this doesn't"

[last iris]

Factorio does

[quick shuttle]

The only other solution we would swallow for ridding startup is adding a deprecated loadEarly method to the config builder and you specific exactly which config entry to load early. However, the changes needed in backend to support this would be immense

[wet hound]

I can't remember a single other game called Minecraft...

[drowsy iris]

What are you talking about henry?

[last iris]

not really. that would be easy. all it needs to do is to prevent the "too early" exception and us loading everything at register but only unlocking access later