You just need a special DynamicOps subclass that is aware of them, and a MapCodec tjay can write them
#Config stuff
cyan needle
visual bobcat
It is basically the major blocker
So if you can make something that works
And has a decently useable API
Then I more then willing to push for this
cyan needle
Mildly overengineered and I haven't written tests yet but https://github.com/lukebemish/codecextras/tree/main/src%2Fmain%2Fjava%2Fdev%2Flukebemish%2Fcodecextras%2Fcomments
sacred rune
fnorge configs aren't exactly a map so much as a mixed list of comments and key-value pairs
cyan needle
That sort of mixed list can be turned into a map without too much issue - given that that's how nightconfig stores it, basically, from what I can tell
visual bobcat
That sort of mixed list can be turned into a map without too much issue - given ...
No they are actually special config entry types
See CommentedConfig.Entry
Which holds the comment
cyan needle
Now, all this said - I'm not sure that codecs are the best solution here. The main reason why is that they're just a bit messier to work with in terms of missing keys and whatnot. It's doable - I have a system for it in fact - but in general if you build a codec with just the normal tools, it'll either not serialize defaults for optional fields (bad for a config) or error on missing fields (bad for a config). In general, it just takes a lot more thought to make a codec that works for a config than a normal one.
If neoforge is handling all that behind the scenes though I suppose it's not an issue
quick shuttle
personally, i feel config codecs are out of scope
at least to a modloader
just enhance the current config system and it'll be fine
visual bobcat
Probabyl true
jovial bane
moar builders
cyan needle
The best use of codecs in configs I think is serializing pieces of a config - providing a default DynamicOps for nightconfig and letting people easily use codecs to serialize values within a config is the direction I think it's worth going in
cyan needle
It's doable but requires the use of a bunch of custom stuff - so yeah, using them for full configs is probably not worth it.
However, for individual fields in a config that doesn't really matter - either you can deserialize the field or not, and if you can't, you recover by just assuming the whole entry is bad and going from there in whatever your outside config logic is - so using codecs for bits and pieces of a config isn't bad
sacred rune
they don't have proper partial error recovery
I just 1) require a valid default value, and 2) log error and fall back to default if the config field is invalid
but yeah I only use codecs for individual fields, not the whole ass-config
vivid surge
a helper like ConfigValue<T> defineCodec(String name, Codec<T>, T defaultValue) would be nice and probably all thats needed
steady tiger
Config stuff
thorny flower
the rename hardly forces discord to refresh it own live while i was reading at the top
and this conversation really concerns me, considering:
no, please no
i come very late to this conversation (but nothing was implemented so is not late yet)
wise ember
no, please no
I think everything but the client config could be loaded with the datapack system
steady tiger
i come very late to this conversation (but nothing was implemented so is not lat...
uhm no i have an entire builder rewrite
stoic cliff
IMO the issue is that it's a builder but without actually being a builder.
like a proper builder API would look like, ```
BUILDER.begin("someField")
.comment("Some Comment")
.defineWithRange(1.0f, 0.0f, 1.0f)
but with the current API, no matter where you put .comment, it's still awkward
jovial bane
we could move towards that style of API without breaking changes
err, I mean, with deprecations and whatnot so it's not breaking current code, and giving a grace period to migrate over to the new code
thorny flower
pull things slower hurts more
steady tiger
jovial bane
_pull things slower hurts more_
ehhh, I'd not say that generally
giving people time to migrate to new code from their old code is a good thing, I would think, rather than ripping things out without a clear path to migrate
sturdy skyBOT
Created Gist at the request of @thorny flower: https://gist.github.com/NeoCamelot/f10e9dc081fe3d88e5c8602d04425163
thorny flower
wait
you really rewrite the....
...
oh wait
you miss float
i see the datapack thing and honestly, if any config entry is "better" placed in a dapaack, use gamerules instead
specially considering how forgotten was gamerules in forge to require ATs to even use it.
thorny flower
honestly i preffer the old one
steady tiger
wat, it's basically the same for users except it has non-primitive values
thorny flower
you're going to want to elaborate on that <:LUL:577908625666867200>
wait
you was here
steady tiger
i know what boxing it, that doesn't explain either of your points
rose trout
The overhead there shouldn't matter, if your reading of config values is performance critical you should be extracting the values into primitive fields in an Event Listener on ModConfigEvent (Loading/Reloading to be specific)
thorny flower
i know what boxing it, that doesn't explain either of your points
you get Number/Boolean as a Number/Boolean
not as a primitive one
when you use it to do basic operations such as 1 + MyConfig.myfield.get() you already do unboxing of the primitive value
now use your config in rendering doing something simple like calculate the distance of one thing or to multiply size of the fog.
stoic cliff
thorny flower
The overhead there shouldn't matter, if your reading of config values is perform...
the event listener is only fired once (at least on forge) and is when config is loaded BY FORGE
stoic cliff
you just have to process the values once in response to a reload event
rose trout
the event listener is only fired once (at least on forge) and is when config is ...
It's fired multiple times
rose trout
.Loading on the inital load
.Reloading when Forge/Neo detects a config change and is reloading the config from disk
thorny flower
my point is avoid that atrocity
lethal bay
you get Number/Boolean as a Number/Boolean
not as a primitive one
when you use ...
You shouldn't be reading a config in a tight render loop. At all. Cache it in a local field you update asynchronously.
jovial bane
volatile
steady tiger
you get Number/Boolean as a Number/Boolean
not as a primitive one
when you use ...
the impact boxing has depends on how you use it. also that's not “unfixable”
rose trout
my point is avoid that atrocity
That's not an atrocity, that's common sense?
lethal bay
~~`volatile`~~
Reading a config in a tight render loop is akin to using volatile 
thorny flower
the impact boxing has depends on how you use it. also that's not “unfixable”
you packed all numbers in one place
thorny flower
Reading a config in a tight render loop is akin to using volatile <:harold:74284...
ignore that for a moment, i forgot to remove it
thorny flower
That's not an atrocity, that's common sense?
re-elaborating
why make that manually in modder side instead of do the primitive catching inside the ConfigValue
lethal bay
Reading a config in a tight render loop is akin to using volatile <:harold:74284...
Though as a point of comparison, it's fine to use something like an AtomicInteger to cache your config value, and then do something like:
while (runTightLoop)
{
var theValue = configCache.get();
/* do the operation */
}
it should technically be unnecessary as any writes of int size and below should already be atomic thanks to x86 memory model, but it's a good practice for instruction pipelining and performance
steady tiger
if you're suggesting 7 classes + overloads with path lists, varargs, default values/suppliers, range checks, that's 10*7=70 utility methods (so current builder but worse).. i digress
thorny flower
default suppliers are already done
lethal bay
In any case, configuration isn't meant to be performance-sensitive, so boxing is fine if it simplifies the implementation
I can see providing things like IntConfigOption or whatever if it's easy to do so, however
steady tiger
and one unboxing per overlay render tick is not the end of the world
lethal bay
and one unboxing per overlay render tick is not the end of the world
Isn't the overlay stuff a FunctionalInterface? You could very easily just cache it on the class
steady tiger
the JIT will do something about it, not to mention the fact that -128 to 127 boxing is cached
thorny flower
lethal bay
Isn't the overlay stuff a FunctionalInterface? You could very easily just cache ...
Something very simple like...
class MyOverlay implements IGuiOverlay
{
private int cachedConfigValue;
public void render(...) { }
private void configurationChanged(ConfigEventIForgotTheName e) { }
public static void register(final IEventBus bus)
{
final var overlay = new MyOverlay();
bus.addListener(RegisterGuiOverlaysEvent.class, e -> e.register(overlay));
bus.addListener(ConfigEventIForgotTheName.class, e -> overlay.configurationChanged(e));
}
}
You could even make it static if you wanted, but w/e
rose trout
the JIT will do something about it, not to mention the fact that -128 to 127 box...
You can also change the positive max for that with a couple of different command line options
Although not really relevant here, it's neat if your have a few values just outside the range
lethal bay
It's an optimisation for specific applications designed for sysadmins, and not really something we can take advantage of in Minecraft
thorny flower
and one unboxing per overlay render tick is not the end of the world
in a modpack the scale of things are important
i still doubt about
but, whaever.
cyan needle
Normally I'm all for avoiding unneeded boxing/unboxing. However, this one... do we have any reason to believe that this is actually causing a performance issue? I've never seen config value accesses show up in a spark report
pale venture
I have seen them occasionally but IMO it's a problem better solved by the few mods that access slow configs in hot paths
quick fable
you can always make config event handlers that copy the config values into static fields
severe sorrel
might be time to revive this
cyan needle
Which stuff in particular?
severe sorrel
fixing the implementation of the configs to remove a number of annoying bugs
zealous goblet
And allowing early loading?
cyan needle
Please, exposing that in a nice way would be amazing
severe sorrel
And allowing early loading?
maybe
it is not my primary goal but can be looked into
steady tiger
so what's your config plan tech
pale venture
the config watcher doesn't even seem to fire reliably on my system in its current state, this seems to be a night config issue not a neo one
might already be fixed in latest night config
the issue is that in FileWatcher$WatcherThread#run() it skips events with a count greater than one, and sometimes the event on my system has a count of 2
yeah seems like the watcher has been rewritten on github
maiden comet
Hello! I wanted to let you know that I'm releasing NightConfig v3.7.0 with many, many improvements (in particular to the FileWatcher, (de)serialization, etc.) 😃
https://github.com/TheElectronWill/night-config/releases/tag/v3.7.0
I've merged the PR by PaintNinja (https://github.com/TheElectronWill/night-config/pull/168). Let me know if you need something else.
maiden comet
(also ew, I really don't like the abbreviation 'serde' for some reason)
sorry about that 😄 It's quite common in other languages though :p (https://docs.rs/serde/latest/serde/, https://pypi.org/project/serde/)
stoic cliff
makes me wonder how no one has been like "uh... that sounds a lot like 'merde' which means shit in french"
anythow 🤷♂️
maiden comet
Honestly I have never thought about that 😮 Pronounced "ser-dé" it's fine I think.
rose trout
Hello! I wanted to let you know that I'm releasing NightConfig v3.7.0 with many,...
*PaintNinja :P
maiden comet
*PaintNinja \:P
oops, thanks ^^
severe sorrel
Nice release, thanks
severe sorrel
ok time to revive this For Real™️
visual bobcat
?
stoic cliff
(no idea I just saw and posted a gif)
tardy swift
Ey yo? What’s the current plan 👀
severe sorrel
I'd just like to fix the small issues that have accumulated
severe sorrel
pale venture
we should bump the nightconfig dep for the new file watcher too
severe sorrel
this is what I have in mind for now:
- Update to the latest NightConfig.
- Make sure that loading and reloading events do not fire at the same time.
- Check reloading with Notepad++.
- Check thread-safety of off-thread reloading. Clarify/check which thread the Reloading event gets fired on in the javadoc.
- Send clientbound packets for changes to server configs.
- Fire ModConfigEvent.Reloading on the client-side when a new config is received.
anything else?
pale venture
anything else?
make sure reloading does not fire in response to the configs being saved during startup
like if there were no changes
in forge packs I had major issues with reloads being triggered just from launching the game when config files already exist
lilac leaf
anything else?
Configs should be done loading by the time the initial resource reload starts
I would have made a PR by now but I didn't want to guess at what approach would be accepted nor did I want to ping all the maintainers or something to try to get a discussion going about it
Related but beyond configs I am still concerned about the timing and naming of setup events
steady tiger
Configs should be done loading by the time the initial resource reload starts
use a startup config for that
severe sorrel
Hey @maiden comet, I have tried to update FML to the latest NightConfig and ran into an issue.
I am loading the config as follows:
final FileConfig fileConfig = FileConfig.builder(modsjson).build();
fileConfig.load();
fileConfig.close();
I use it as follows: (simplified)
fileConfig.getOrElse(List.of("dependencies", "testproject"), ArrayList::new);
This getOrElse call fails to retrieve the inner config
I have traced the issue to this place
level is a SimpleCommentedConfig so it doesn't get read further
am I doing something wrong?
maiden comet
Hey <@163947104476200960>, I have tried to update FML to the latest NightConfig ...
Does dependencies.testproject come from the file or from somewhere in the code?
severe sorrel
it's from a file
I think I found the issue
convertConfig doesn't seem to convert arrays
I am trying to fix it 😄
maiden comet
ah, that's possible 😄 Maybe we can reuse the functions of StampedConfig, I'll take a look.
putAll seems to work better than the duplicated conversion function, I'm writing a new test for that 🙂
severe sorrel
`convertConfig` doesn't seem to convert arrays
this might be related, but I find the use of into on line 286 suspicious
maiden comet
it's from TestParser#parseInto IIRC
maiden comet
I'll modify it to test for arrays, that was the missing case
maiden comet
Could you share the config file that you've tested so that I add another test with it? Just to be sure that it works now ^^
severe sorrel
Could you share the config file that you've tested so that I add another test wi...
modLoader="javafml"
loaderVersion="[3,)"
license="CC0"
[[mods]]
modId="testproject"
version="0.0.0"
displayName="Test Project"
description='''A test project.'''
[[dependencies.testproject]]
modId="neoforge"
type="required"
versionRange="[999.6,)"
ordering="NONE"
side="BOTH"
there
maiden comet
thanks!
maiden comet
Fix pushed, but Sonatype deprecated the password authentication 🫠 I need to find the right gradle config in order to publish the new release
severe sorrel
this looks saner than what we have right now
severe sorrel
slow progress... I'm starting to add some tests for our config handling in FML while I rework the code
severe sorrel
yay the first batch of tests is working...
vivid surge
3s to open a file 
severe sorrel
that's just because it's first test to run
oak crescent
can't you run a setup task before that?
severe sorrel
No idea
jovial bane
@BeforeAll?
or @BeforeEach
iirc those annotations exist in JUnit (assuming this is JUnit, though other test frameworks have their equivalents)
severe sorrel
I already have some logic in BeforeEach
Honestly idk why the first one is slow but it doesn't matter
severe sorrel
hey @maiden comet, it looks like the filewatcher will shut itself down as soon as one directory gets removed
shouldn't this break be a continue instead?
I am writing some unit tests for our config handling, and ran into that... only the first test has a functioning watcher since the temporary directory gets closed at the end of the test, bringing down the entire watcher 😄
severe sorrel
@mystic temple I see that you subclass ModConfig (https://github.com/mekanism/Mekanism/blob/1.21.x/src/main/java/mekanism/common/config/MekanismModConfig.java)
is that something that needs to be kept?
I want to make it final
(almost every use I can find on github is the little redirection hack for server configs; of course that doesn't apply anymore)
an alternative for Mekanism would be to store a static Map<ModConfig, IMekanismConfig> I think?
mystic temple
Unsure. Now that I don’t need to be overriding to allow for a custom redirect of folder. I could probably just move the constructor bit to the helper method. The other bit is basically so that I have a way to access the backing object from it, but given I have to check the modid anyway, I can probably move it to a map, yeah
severe sorrel
or making a custom IConfigSpec since I am revamping that interface to not suck (it will be easy enough to forward to ModConfigSpec)
mystic temple
Main reason that subclass existed was for the folder redirect hack I used to have to do
maiden comet
I am writing some unit tests for our config handling, and ran into that... only ...
I'll take a look rn ^^
maiden comet
publishing new release soon, good catch!
severe sorrel
Thanks 🙂
severe sorrel
not released yet? 😄
severe sorrel
the threading model to safely edit configs and stuff will need some thinking
most likely, every access to a config will have to acquire a lock
maiden comet
in addition to the lock that already exists in new FileConfigs? or is it this one that you want to acquire
maiden comet
not released yet? 😄
now it is 🙂
severe sorrel
Cool thanks 🙂
severe sorrel
in addition to the lock that already exists in new FileConfigs? or is it this on...
Not sure. I need to think about which threads might be reading and writing a config
severe sorrel
I suppose that this breaks something because the watcher is not receiving notifications anymore
https://github.com/neoforged/FancyModLoader/pull/172/files#diff-0872b4f1133801d42f109ee474e18df19a12efc232d53924a65d54e8d8cb5646R143 this test works because it goes first
then testReload fails because the watcher doesn't receive notifications anymore
interestingly, running only testReload succeeds
so it has to be a problem somewhere™️
severe sorrel
It's still not happy if I don't unregister the watcher 😅
ok, this error happens when the directory gets deleted before the file watcher even starts watching it xD
I don't think it's really fixable
ok I get the reason why my test is failing now...
the file is modified before the file watcher is registered 😐
last iris
I was a bit bored but unmotivated tonight, so I worked on my proof-of-concept config GUI. A couple hours later and it can handle Strings, Ints, Longs, Doubles, Enums, Bools, displaying and editing the values of Lists of those (only String and Int tested, deleting/adding/reordering entries missing). https://gist.github.com/HenryLoenwind/216f602f78b310064446364e976de127 I would say it looks promising and a couple of quirks could easily be corrected by editing the config classes.
Edit: updated
Edit 2: Draft PR created
severe sorrel
I was a bit bored but unmotivated tonight, so I worked on my proof-of-concept co...
let me pin that so we don't forget about it
I don't want to have a look now, but adding back a basic config gui would be awesome so let's try to make it happen 😄
last iris
np, I'm primarily posting so I have a reason to stay on this tomorrow. ;) Stuff that's just sitting on my hard drive with nobody knowing about it get forgotten quickly
severe sorrel
@maiden comet I would need a way to register a file watcher and block until the watcher is registered; is that possible? I want to make sure that the watcher is registered before I even read the file, to make sure that I will catch all modifications eventually.
maiden comet
Not sure. I need to think about which threads might be reading and writing a con...
it shouldn't matter which thread creates the config and which thread uses it. What's important is that if you want to do multiple reads or modifications at once, you use the bulk<OP> methods (such as bulkCommentedUpdate)
maiden comet
<@163947104476200960> I would need a way to register a file watcher and block un...
yes it should be possible! Let me try to introduce a new method with a barrier of some kind to wait for the registration 🙂
severe sorrel
If you can make it return a CompletableFuture or similar I'd be able to setup the watchers for multiple configs in parallel which would be good
maiden comet
writing tests for FileWatcher::addWatchFuture which returns a CompletableFuture 🙂
(maybe some of you will find a better name?)
severe sorrel
Well addWatch could always return a CF but that would be a breaking change
maiden comet
Yep, for a 3.x version I'll keep the old methods as is
maiden comet
it shouldn't matter which thread creates the config and which thread uses it. W...
I've made FileConfig extend ConcurrentConfig to help with that (you can handle them like non-file concurrent configs)
severe sorrel
Yeah I know that the configs themselves won't crash with multi threaded access. But since we need to maintain some additional state I have to think about it 😉
maiden comet
ah yes, in that case that's a legitimate question to worry about!
maiden comet
night-config 3.8.0 has been released
jovial bane
I assume that's meant to be a draft, since it still relies on an as-of-yet-unreleased version of FML
severe sorrel
Yes
It's not a draft because it probably needs to be reviewed at the same time as the FML PR
Also: I spent so long on this annoying config refactor that I want to get this merged even though it's not perfect yet. At least we (or at least I) will be able to fix further issues now that I understand the system.
jovial bane
you'd still want to make sure the FML PR is pulled before the NF PR 
hopefully I don't make that mistake
severe sorrel
that's what the required checks are for! 😄
jovial bane
severe sorrel
I think I can find a workaround tomorrow
however note @maiden comet that calling Config.copy(config) on an arbitrary config will fail for a StampedConfig 😅
and in general, code that works with Config might not work if that config is actually a ConcurrentConfig under the hood for that same reason
maiden comet
however note <@163947104476200960> that calling `Config.copy(config)` on an arbi...
oh, I should special-case ConcurrentConfig for copy in that case
maiden comet
<:ohno:954846725426806845>
StampedConfig will propably never support valueMap(), it's hard to return such a Map while preserving the thread-safety and multi-readers property of the underlying config... (IMO the "original sin" was to provide valueMap() in the first place 😄)
maiden comet
maybe I can still provide a limited but sufficient version of it though
severe sorrel
I don't think it should be implemented, it wouldn't make sense
Unfortunately this creates the annoying situation where some configs need special handling
severe sorrel
ok, FileConfigs are now ConcurrentConfigs, this makes my life a lot easier 😄
severe sorrel
update: I lied, my life is hell again 😭
maiden comet
update: I lied, my life is hell again 😭
why 😢
severe sorrel
I find it very hard to reason about which properties a specific config might have
for example if I do concurrentConfig.bulkRead(c -> { c.valueMap() ... }) and the map contains a Config, is that a concurrent config or not?
maiden comet
- Why do you need to know that?
- Why are you using valueMap instead of entrySet? 👀
- To actually answer, yes it is, because both SynchronizedConfig and StampedConfig convert all subconfigs to the same type (for thread-safety reasons)
severe sorrel
oh there is an entry set
so the reason why we need the map, apparently, is this cursed API that we have:
public interface IConfigurable {
<T> Optional<T> getConfigElement(final String... key);
List<? extends IConfigurable> getConfigList(final String... key);
}
there are some <Map<String, Object>>getConfigElement calls...
severe sorrel
1. Why do you need to know that?
2. Why are you using valueMap instead of entryS...
thanks for 3. This means that I can at least do bulkRead(c -> Config.copy(c)) to not leak the "concurrency" to code that doesn't need to know about it 😄
hmmm no, can't even do that The view provided by bulk operations on StampedConfig does not support valueMap()
what is the simplest way to make an immutable copy of a stamped config?
for an example of valueMap usage, have a look at nightconfig's own ConfigSpec 😅
NightConfig itself seems to be using valueMap in a lot of places
severe sorrel
hmmm, Config.copy does a shallow copy :/
maiden comet
there are some `<Map<String, Object>>getConfigElement` calls...
but config values are almost never Maps, how does this possibly work 🤔
severe sorrel
if (value instanceof UnmodifiableConfig) {
return (T) ((UnmodifiableConfig) value).valueMap();
I have a library that I use to put maps in my toml configs (by reading sub-Configs as maps)
severe sorrel
that's how it works 😅
maiden comet
```java
if (value instanceof UnmodifiableConfig) {
return (T) ((...
and for sub-sub-configs which will end up as Config values in that Map?
severe sorrel
either they are not used, or someone has to cast them I suppose
I personally think it's bad design but it's what I have to work with 😅
maiden comet
at least a conversion view or TransformingMap could be used to handle those automatically
but okay, it seems that valueMap() is still needed somehow, I can implement it for StampedConfig (for "simple" use cases only)
maiden comet
hmmm, `Config.copy` does a shallow copy :/
Would you need a way to create deep copies? There's an issue for that, I didn't make much progress on it, but since I'll have to fix Config.copy(otherConfig) for ConcurrentConfigs, I can do that as well ^^ https://github.com/TheElectronWill/night-config/issues/132
severe sorrel
I strongly dislike wrapping views, I find it makes the code very hard to understand 😅
so my fix is to turn the StampedConfig into a plain immutable config once
maiden comet
so mod configs are readonly? (it's been a while since I wrote mods 😅)
severe sorrel
neoforge.mods.toml is
mod configs are not readonly, but we don't give direct access to the Config object so there I can hide the complexity
maiden comet
okay
severe sorrel
apparently mod configs are using SynchronizedConfigs, that's why I didn't find out about the issue earlier
maiden comet
makes sense, they probably use sync() when they are built
severe sorrel
they do
maiden comet
my bad for not having enough tests with async() then 
severe sorrel
nice emote 😄
drowsy iris
Config stuff!
So, I am about to make it a read/write-lock
thoughts?
(and funnily enough, why is openConfig ultimately not accessing any state in ConfigTracker?)
severe sorrel
So, I am about to make it a read/write-lock
instead of a reentrantlock?
drowsy iris
instead of a reentrantlock?
We ultimately just need to access it sequentially when adding configs, later on it's unnecessary
severe sorrel
compared to lock-free stuff at least
eventbus used to have readlocks and they weren't great
drowsy iris
Well we ain't gonna get lock-free
severe sorrel
for what exactly is this?
drowsy iris
it replaces all the synchronized inner collections
pale venture
if this is being written a couple times on startup consider a COW structure
drowsy iris
Whenever you access fileMap, configSets or configsByMod
severe sorrel
if this is being written a couple times on startup consider a COW structure
read performance is not a concern
ah
well it really doesn't matter
drowsy iris
Eh I suppose you're right I can leave it
it's a bit of a footgun to see this:
this.configSets.get(type).forEach(config -> openConfig(config, configBasePath, configOverrideBasePath));
And realize it's actually necessary to use forEach here
To keep it thread-safe
Using a normal for-loop would require additional synchronization
severe sorrel
ahah you're right; I didn't even notice
drowsy iris
but you're right, I'll just revert ^^
drowsy iris
Yes I already commented
😄
Then removed my comment again after starting to impl a writelock, lol. But I'll fix that
Ahem
So, fun observation
openConfig, closeConfig, acceptSyncedConfig, resolveBasePath can be static
severe sorrel
yes
drowsy iris
They all take a ModConfig as their first param, should we just move that over there?
severe sorrel
I'd rather keep all the messy stuff in a single file
drowsy iris
Ugh what a design 😄
severe sorrel
and yeah it's annoying; it would be a lot better to have it all in neoforge
(guess how it was in 1.16... oh well)
steady tiger
just throw internal on all classes, it's fine
drowsy iris
openConfig is not used in Neo
Thoughts about at least moving openConfig / closeConfig into ModConfig and making them packag-visible?
Oh fuck my life, why is that used from ModContainer (openConfig)
steady tiger
startup configs?
severe sorrel
yes
severe sorrel
Thoughts about at least moving openConfig / closeConfig into ModConfig and makin...
I have it setup this way so that we separate the ugly impl from what is modder-facing API
and yes, everything public is used from another package
drowsy iris
protected final EnumMap<ModConfig.Type, ModConfig> configs = new EnumMap<>(ModConfig.Type.class);
Urghl, it's essentially only so that the config is in this map on the container, before a startconfig is loaded
drowsy iris
well its protected, so I suppose language loaders could grab it
Ours don't
I doubt the kotlin or MDG ones do
severe sorrel
we could just make it a method in ModConfigs
because the config screen PR is currently grabbing it via reflection
drowsy iris
lol the config map?
severe sorrel
yes
wouldn't it be nice if we could inject a neoforge-provided configuration container into the mod constructor? 😛
drowsy iris
pushed minor refactor
Submitted review comment. The watcher registration & deregistration is async, but he also has apis that allow us to wait
drowsy iris
What is the reason for the classloader change in ConfigWatcher?
severe sorrel
in general?
we generally want the transforming CL to be the context class loader on any thread that will invoke mod code
(this was essential in pre-neo eventbus)
drowsy iris
Alright, then I will swap it out only for the mod call though...
and swap it back via try/finally
drowsy iris
@severe sorrel okay, so looking at the config watcher
I think the order of events should be:
- Obtain the mod config lock
- Register the watcher and wait
- load the initial config
- release the lock
Currently you're not obtaining the lock at all, which means the watcher can load the config concurrently with the initial config load, right?
severe sorrel
yes
drowsy iris
I'll try to fix that
severe sorrel
this is a good use case for virtual threads, btw
I don't want to wait for 200 futures sequentially
drowsy iris
synchronized collections aren't great for virtual threads, tbh
severe sorrel
hmmm yeah
severe sorrel
the VTs don't need to hold the lock
drowsy iris
I think in reality we don't need to wait
severe sorrel
we only need to assign a thread per ModConfig
drowsy iris
If we lock it
because then the watcher cannot race us to reload before we're done anyway
severe sorrel
so: the reason why I didn't do any of that is just that it doesn't matter in practice
drowsy iris
and any deregistration of the watcher will go to the same background thread queue
severe sorrel
the config watcher is already a niche feature
drowsy iris
It should still not race a reload event before the initial load
that shit is terrible to debug
severe sorrel
the problem is if the config gets modified between us loading it, and the watcher being registered
drowsy iris
the lock fixes that easily
severe sorrel
no
severe sorrel
you need to wait for the watcher's registration, and only then even try to read the file, to truly fix it
severe sorrel
oh you mean if we do NOT wait
yeah
severe sorrel
It should still not race a reload event before the initial load
hmmm yes
that could happen technically
unless we just take the lock before registering the watcher
drowsy iris
that is what I plan to do
take the lock
register the watcher
do the initial load
release the lock
severe sorrel
👍
drowsy iris
but your point about performance if we wait is valid
What about this?
var futures = new ArrayList<CompletableFuture<?>>();
this.configSets.get(type).forEach(config -> {
futures.add(CompletableFuture.runAsync(() -> openConfig(config, configBasePath, configOverrideBasePath)));
});
CompletableFuture.allOf(futures.toArray(CompletableFuture[]::new)).join();
this should run on the ModWorkManager#parallelExecutor to ensure that we have the correct CCL
drowsy iris
Ah good point
severe sorrel
otherwise yeah it's a good way to go about it
multiple Loading events might fire concurrently for a given mod
is that a problem?
drowsy iris
Is the directory watcher serial?
The lock is only per config, not per mod
I suppose we could actually inject the lock per mod
severe sorrel
right now all watchers run on the same thread
but it's more of an impl detail I think
drowsy iris
Hmmm, we currently do not really dispatch concurrently? Render events, maybe?
severe sorrel
you mean within a mod?
it could happen if something happens on the client and the server at the same time; otherwise no
last iris
because the config screen PR is currently grabbing it via reflection
no, I switched to ConfigTracker.INSTANCE.configSets().get(type)
severe sorrel
ah good
last iris
there actually is a way to access it, it just was a getter at the very end of a class. Probably missed it because (a) I expect getters to be above methods with code and (b) when scrolling down a class, missing a 3-liner at the very end...
drowsy iris
OOooOooh wait, Tech
We will not dispatch in parallel
since there is only one config per type per mod
severe sorrel
really?
severe sorrel
there actually is a way to access it, it just was a getter at the very end of a ...
that's why it's now getting split to ModConfigs
last iris
any clean way to get it is fine with me.
drowsy iris
It's a Map<String, Map<Type, ModConfig>> though
last iris
the thing I hate most about the system is new ConfigurationSectionScreen(context, this, subconfig.valueMap(), key, subsection.entrySet()) Havint o traverse two tree is parallel for the valueSpecs and the ModConfigSpec.ConfigValues is annoying
severe sorrel
these are the only uses of that map
I think that we should generally allow multiple configs of a given type
severe sorrel
the thing I hate most about the system is `new ConfigurationSectionScreen(contex...
I don't understand why the specs don't store the value
last iris
yes, especially big mods traditionally split their configs because they have so many values.
drowsy iris
but yeah it is technically not enforced but then that api makes little sense
last iris
indeed. I considered adding it, but then I would still need to traverse the other tree for levelComment etc., so it wsn't worth it
severe sorrel
but yeah it is technically not enforced but then that api makes little sense
I think we can change that API; it's quite niche
drowsy iris
So, what do we do about the parallel dispatch. I feel like this might be unexpected for people
last iris
would be nice, I can then change the top-level screen from listing just the 4 types to listing the configs and using the types a subheaders. would look way nicer. At least if those configs had names...
severe sorrel
So, what do we do about the parallel dispatch. I feel like this might be unexpec...
make the locks per-mod?
severe sorrel
I can do it
severe sorrel
probably work for tomorrow
yes
drowsy iris
I'll commit what I have anyway
pale venture
the problem is if the config gets modified between us loading it, and the watche...
IMO you shouldn't worry about such an unlikely case
drowsy iris
this will happen
pale venture
this *will* happen
saving the config again will then reload it, no?
last iris
the 110% way of working with a watcher is having one that fires when it's being added because it sees being created as a change
drowsy iris
saving the config again will then reload it, no?
The problem is that your mod gets config reload events before it gets config load events
who knows what weird unreproducable crashes that might bring
severe sorrel
@drowsy iris you are blocking the threads inside the futures, not good!
calling Future#get inside a future -> bad
we also don't want to slow down mod init massively by blocking on startup config creation
the only real solution would be to move mod initialization to virtual threads, I think
but this is getting out of scope 😅
drowsy iris
<@84313704874663936> you are blocking the threads inside the futures, not good!
hm? am I? 😄
severe sorrel
drowsy iris
well it's still pooled 😛
severe sorrel
meh; STARTUP configs will also suffer from this
drowsy iris
their CTORs run already in parallel
severe sorrel
we should run them on virtual threads if we want to preserve throughput
drowsy iris
i mean the other option is to just fucking ignore the config change between loading it initially
and the watcher coming up 😄
severe sorrel
yes that's what I want to do for now
drowsy iris
yes do it
severe sorrel
👍
drowsy iris
there are other options btw
such as: record the last modified date
and register a completion handler to the watcher registration
that does a reload if it changed
severe sorrel
my preferred option would actually be to register the entire config directory for watch events and implement the watcher manually
since we know that all events come from config/
(+ the world config folder, maybe)
drowsy iris
The fun part is that it's dir watchers anyway on windows 😄
But yeah, I'd vote for knowingly ignoring a config change that happens between initially loading the config and registering the watcher
we can fix that later by doing a last-modified check via a future completion handler
to handle the case manually
severe sorrel
yeah that would also work
last iris
why not throw the modification notifications into a list (or a Set) and process them later one by one? Pause that processing until the main menu comes up and then process one per second (with an "oops, someone just copied in a complete config folder, process everything at once" check if that Set has more than 10 entries). I don't think anyone would complain about a second or three of delay when changing a config file...
severe sorrel
the processing is already off-thread
the issue is the following:
- we ask nightconfig to register the watcher
- we read the config
- the user modifies the config
- the watcher actually gets registered
if that happens, we will never receive a notification from the change in step 3
drowsy iris
btw if you reorder it back such that we register the watcher after loading the initial config you can remove the locking too
^^
check my other small changes as well
vivid surge
the issue is the following:
- we ask nightconfig to register the watcher
- we re...
why would people be editing the config whilst the game is booting/world is loading (when they should know by the configs name (client/server/startup) that the file is being loaded around that time) and expecting it to actually work? it seems like common sense 'dont modify a file while its being read'.
drowsy iris
are we good? <a:blobsweat:834480454945865728> check my other small changes as w...
will do
drowsy iris
why would people be editing the config whilst the game is booting/world is loadi...
Oh auto-restarting servers, you change a config while it's just restarting, then wonder if it actually took the config or not
But it not recognizing the change in that situation is better than an out-of-order event notification (first reload, then load)
drowsy iris
are we good? <a:blobsweat:834480454945865728> check my other small changes as w...
I think it looks good. One question I had which doesn't really relate to your PR: we currently post a reload event for every change notification of the watcher, right?
drowsy iris
Well no
The file watcher can create spurious notifications
generally
The question I have is: we do not seem to check that a config after reloading has actually changed vs. the previously loaded one
severe sorrel
indeed
and the actual config is mutated directly, so you couldn't really check with the current design 😛
drowsy iris
blergh
I am not gonna hold the PR for that, but generally I'd expect we copy the old one, reload, compare and only emit a reload event if anything has actually changed
drowsy iris
Hehehehe
severe sorrel
it will be easier to send piecewise improvements
drowsy iris
Approved
severe sorrel
now I need to fullfil my promise and go review the other FML PR 😄
drowsy iris
heh 😄
severe sorrel
pssst @drowsy iris can you have a look at https://github.com/neoforged/NeoForge/pull/1239
drowsy iris
oh my
mystic temple
I will try updating mek to that tomorrow and seeing how much it breaks 
drowsy iris
It should™️ not break anything
sturdy skyBOT
pupnewfster referenced
drowsy iris
Ooopsie
jovial bane
this reminds me of a feature I should get in, to add a space character between the comment # and the actual comment text
jovial bane
I had worked around it by extending the spec builder myself
https://github.com/sciwhiz12/Concord/blob/f925a4a73785c95d7eece5ac77ae9f87f7e8b7aa/src/main/java/dev/sciwhiz12/concord/ConcordConfig.java#L284-L319
last iris
btw, what's the clean way to do IConfigEvent.reloading(ModConfig).post(); now?
ModContainer.acceptEvent(new ModConfigEvent.Reloading(ModConfig));? Or will it be posted automagically by ModConfig.save(); because of the watcher and I can skip sending the event from the UI?
last iris
ah, so we're at a state where the old way has been removed and the new isn't in yet. Guess I'll leave my PR at "not compiling" then until the next one is merged.
mystic temple
Am I correct in my skimming that we now properly support having multiple configs of the same "type" (for example multiple server configs)?
Edit: Seems partially maybe??? ModContainer#addConfig still doesn't support it, but similar to before ConfigTracker#registerConfig seems to support it
also how are we meant to get a modconfig? Given instead of calling #save on the config spec, it says we should call onConfigChanged on the mod config. But ModContainer#registerConfig doesn't return the ModConfig object that it registered, so you can't capture it there. Where should we be capturing it?
severe sorrel
Am I correct in my skimming that we now properly support having multiple configs...
Yes!
mystic temple
but
protected final EnumMap<ModConfig.Type, ModConfig> configs = new EnumMap<>(ModConfig.Type.class);
that isn't a map to list
severe sorrel
😦
mystic temple
config tracker supports it
but it always seemed to support it
that's why I previously manually registered stuff with the tracker and sometimes skipped adding to the container
but now it seems like to make sure everything gets hooked up properly I need to go through the container
severe sorrel
Oh the ModContainer field is pointless anyway
severe sorrel
Yeah
Now you say we also don't properly return the ModConfig hmmm
You are correct
Maybe I'll implement a slightly different design then to make ModConfigSpec#save work nicely
mystic temple
also, you mentioned previously there is now a cleaner/possible way to create custom specs? Or is that not quite true given while you can extend mod config spec, the builder then is a bit clunky to try and extend, and I really should just basically make a map that uses IConfigSpec as the key (as opposed to say ModConfig) and then just grab the spec from the mod config in the reloading/loading events etc
last iris
I have one tiny feature request: Either
(a) filter the filename to disallow any characters that are not valid in a translation key, or
(b) add a translation key parameter to at least the register method that takes a filename.
I'd prefer not having to fight with the ModConfig object to coerce it to give me something that might be used in the UI...
mystic temple
hmph, having to reflect to access ConfigTracker.configSets, but that isn't too bad, given my only use case anyway is to bootstrap common and server configs in datagen, as I do validation of certain blocks and stuff, which then to construct some bits, but more so to query other things on the objects that normally wouldn't be queried during startup, it needs the configs to be initialized, even though I don't actually care about the value of said configs
severe sorrel
The access via ModConfigs is not enough?
last iris
if you just want to bootstrap them in datagen, couldn't you re-register them as STARTUP in your datagen code? I see nothing stopping you from doing that at any time. They're not sealed like registries...
severe sorrel
also, you mentioned previously there is now a cleaner/possible way to create cus...
I think I would in general prefer extending the functionality of ModConfigSpec, since the code is quite tricky
severe sorrel
I have one tiny feature request: Either
(a) filter the filename to disallow any ...
Hmmm that will need more thinking
mystic temple
I think I would in general prefer extending the functionality of ModConfigSpec, ...
how? Like most of the builder methods don't return a ModConfigSpec, but to return something of the type ModConfigSpec from build might be annoying? Or do you mean create a wraper for ModConfigSpec that bounces all the methods, and then adds whatever extra functionality. And stores a ModConfigSpec as the internal thing it wraps, which is produced from building the builder? Andif so we may want to provide utility wrappers for mods to make use of then (similar to how we provide a baked model wrapper)
severe sorrel
What exactly do you need that ModConfigSpec doesn't provide?
mystic temple
I want to store a reference to another object, so that on config event I can call a method on said object
a simple Map<Key, Object> wth lookup would technically suffice
to just attach data to the spec
that I then can consume again later
severe sorrel
Ah yes I remember now. The map is probably the simplest option yeah
mystic temple
I will do some testing later, but other than the fact that ModConfigSpec#save is deprecated for removal (because it doesn't actually fire the change events). I think I have everything compiling now?
Using reflection to access configSets in my datagen module, but that is just details and not code that gets shipped to end users anyway
last iris
Map<ModConfigSpec.ConfigValue<?>, Consumer<ModConfigSpec.ConfigValue<?>>> callbacks = ...
and then use a static passthrough method that puts your configValues into that map and returns them for assignment
severe sorrel
I will do some testing later, but other than the fact that `ModConfigSpec#save` ...
Okay. Thanks a lot for your feedback!
mystic temple
But yeah only blocker imo is the save thing or at least some way to get the mod config so that the replacement can actually be called
(Assuming it actually works when I launch the game and try to enter a world)
last iris
btw I'll wait with adapting my PR until the neo part is merged. I have plenty of changes to ModConfigSpec I'll need to un-merge-conflict whereas the fml part is half trivial and half fun. Need to combine the annoying and the fun ;)
drowsy iris
yes
severe sorrel
with this we might even be able to get entirely rid of the FileConfig
mystic temple
I want to do this
This looks like a good solution to make save be able to fire the event properly
steady tiger
do I get to complain about config tracker doing the weird INSTANCE pattern
you're cleaning up, right? :P
severe sorrel
do I get to complain about config tracker doing the weird INSTANCE pattern
I am actually kinda relying on that for the config tests
severe sorrel
you're cleaning up, right? :P
I'm going through your comments on the NF PR right now; wdym?
steady tiger
I am actually kinda relying on that for the config tests
it's a.. psf
pls tell me you're not doing what I think you are
severe sorrel
hmmm?
steady tiger
ah you're just creating a new tracker instance and using that
severe sorrel
yeah
steady tiger
eh, whatever
you're also doing exists checks before reading
with nio. that should just be reading that catches a file not found
severe sorrel
yeah I considered it
steady tiger
your fml pr got a nitpick, a design question and a suggestion to consider that again
severe sorrel
I just pushed a reconsideration 😛
I like exposing a minimal API, hence the sealed interface
steady tiger
you're really against readable javadocs smh
severe sorrel
tbh I orginally had it as a NonExtendable interface then remembered that we are on modern java
I actually find <p> on the same line more readable
blame years of Fabric triage
severe sorrel
I like exposing a minimal API, hence the sealed interface
I don't see a need to expose the path or the modconfig atm
steady tiger
blame years of Fabric triage
don't worry, we'll fix you in time
severe sorrel
I've been living in Germany for 3 months 😛
steady tiger
can I get you to review the NeoForge PR as well so we can ship this? <:yeet:7227...
my review would be pointless right now, you need to update the pr smh
steady tiger
I've been living in Germany for 3 months 😛
you can still resist it
drowsy iris
I've been living in Germany for 3 months 😛
bitteschön
severe sorrel
neo PR updated
last iris
Exception message: java.lang.RuntimeException: Config conflict detected!
I know what I messed up with my temp test, but maybe we could get a message here that's a bit more helpful for modders?
severe sorrel
Does this happen when you register two configs with the same filename?
last iris
yes. or without a filename, in this instance. I changed one of the configs to be the same to test my rendering with two configs of the same type and forgot to add a filename parameter.
It took me a couple of seconds to realise what happened, but I guess a typical modder would be just puzzled and would have to search for the cause
Two configs with the same filename detected or Duplicate config file something like that would be better
severe sorrel
Most importantly it should give the offending file name
last iris
or at least the modId, you're right, I didn't even notice how sparse the thing actually is
OT: Why are kids so stupid to state their actual age on their profile pages of services they're clearly too young to use? I swear, we weren't that dumb at that age back in my time... (re 1282, the tlauchner guy...)
steady tiger
@last iris 
this UI seems wrong
last iris
What about it? Neo creates exactly one section at the top level of each of its configs. Or do you mean the tooltip? Should I remove the dividing newline if the tooltip text is empty?
steady tiger
well yes the tooltip looks wrong too
but the button is off centre
it's weirdly padded to the right
last iris
it's a 2 column layout, there's just nothing there to show it with that weird single-top-level section
I have contemplated removing those sections, but that would break existing config files. we're still in beta, so I could do it...
steady tiger
well
mods will have those kind of categories too, they ensure backwards compatible organization
can you not check that the section is empty?
just opening the config screen class in intellij is throwing an ungodly amount of nullity warnings at me
(those errors are because it still thinks it's java 17... screm)
last iris
I could, and I do that at the top-level, but it gets more and more complicated the deeper you go down the configuration tree
do we have a warnings setup for eclipse, I can take care of those if it matters. So far I've been running with default warnings
steady tiger
i don't think eclipse can recognize the nullity annotations in mc and jetbrains' annotations
mind if i push a bit of cleanup to your branch?
last iris
depends on what your definition of cleanup is ;). If its null annotations/formatting/namings and the like, I'd be happy for you to do that. if it's functional changes, I'd prefer to talk about them first.
steady tiger
it's annotations and converting some else if chains to swithces
last iris
I tried to comment whenever I thought the "why" behind the code wasn't clear, but "tried"...
um, what can be switch()ed that isn't? Has java extended switch() to instanceof or something? I have to admit, I have not followed Java development for a while. Work has always been horribly conservative with Java versions...
steady tiger
yes
last iris
but sure, switch() usually is a good thing
huh. they really have changed their stance on things lately. instanceof has been a dirty word for a long time...
steady tiger
elements.add(switch (cv) {
case ModConfigSpec.BooleanValue value -> createBooleanValue(key, valueSpec, value, value::set);
case ModConfigSpec.IntValue value -> createIntegerValue(key, valueSpec, value, value::set);
case ModConfigSpec.LongValue value -> createLongValue(key, valueSpec, value, value::set);
case ModConfigSpec.DoubleValue value -> createDoubleValue(key, valueSpec, value, value::set);
//noinspection rawtypes
case ModConfigSpec.EnumValue value -> createEnumValue(key, valueSpec, value, value::set);
default -> switch (valueSpec) {
case ListValueSpec listValueSpec -> createList(key, listValueSpec, cv);
case ValueSpec spec when spec.getDefault() instanceof String -> createStringValue(key, valueSpec::test, cv, cv::set);
default -> createOtherValue(key, cv);
};
});
last iris
looks fine. takes a moment longer to read with the default instead of an outer else, but that's fine
edit: and that is not the former outer else. oops, I misread that ;)
"" -> old habit from pre-Java times...
but it has the advantage over .toString() that it's null-safe ;)
btw, I just see that just below
final Object object = context.valueSpecs.get(key);
if (object instanceof final UnmodifiableConfig subconfig) {
"object" should be inlined. I missed that when cleaning up temporary assignments.
steady tiger
case UnmodifiableConfig subsection when context.valueSpecs.get(key) instanceof UnmodifiableConfig subconfig -> {
elements.add(createSection(key, subconfig, subsection));
}
that works too interesting
but it's not equivalent
last iris
yes, that check for undoable elements...
steady tiger
// TODO I have no idea. Help? this is a great comment
last iris
I did note it in the PRs description ;)
steady tiger
but it's not equivalent
hmm actually it should be
if the condition doesn't pass it will fallback to the default case which has a default impl of returning null
which will end up being filtered out
last iris
the point is that neither createSection() nor createOtherSection() are undoable, so they shouldnot enable the undo button, but the create...Element() ones are if they return != null
steady tiger
hmm?
last iris
and btw, the unhandled else case for createSection() doesn't matter. That could flow into other section, too.
steady tiger
that's exactly what i said :p
last iris
oh, you wrote that while I was typing, so I missed half of it. sry
steady tiger
it's fine, i was confused on the relevance of createElement in this case
last iris
yes. elements are editable things that can be undone, sections are buttons that open another screen with its own undo logic
steady tiger
this whole thing could also get rid of the null check, and just just treat a null value spec as returning null, right?
last iris
technically createList() is a section, I just didn#t want to special-case it to not make the code too convoluted
looks good
steady tiger
i see you had fun with numeric values
last iris
fun would have been patching something into Number...that class is surprisingly useless for its name
steady tiger
any reason you're using decode over parseInt/Long?
last iris
but at least I did manage to have all Number values handled by one copy of the code
steady tiger
i suppose for hex representations for colours or something
last iris
something about different error handling
I don't quite remember, other than that I selected that one after comparing the choices I had
steady tiger
also possible bug
in ConfigurationListScreen#rebuild
the create methods have the possibility of returning null but you're alway sincrementing the index
last iris
yes, because that's the array index, and that increases even if a null (or something I don't have an UI element for) is in the array
steady tiger
ahh
last iris
I could have used for(;;) to loop over the list, but I hate that one. no sane language should copy that old C-loop syntax... ;)
steady tiger
well i changed it to that because it's more efficient if you need the index
last iris
for a (0 .. #size)is so much nicer, isn't it? ;)
steady tiger
the "new" for loop compiles down to an iterator
last iris
btw if you're optimizing this, you can combine the two loops and get rid of èlements` here. as each element gets paired up with a label, it's not needed here
steady tiger
i noticed that too
last iris
that elements is only there because addSmall() pairs its parameters
steady tiger
last iris
and uh, I removed the "vanilla style" option from section, which means that could be combined now, too.
last iris
in switch (needsRestart) { that return is needed otherwise the super call will overwrite the setScreen()
steady tiger
okay this is annoying
the current version of the eclipse formatter spotless uses will cause it to scream when when clauses are used...
steady tiger
and downgrading it to 4.31 works fine except it reintroduces that new line thing with records
yes it's ANOTHER DIFFERENT SPOTLESS BUG
drowsy iris
Ah yes, right, the one I am thinking of is their Google Formatter just Crashing on J21 code
severe sorrel
Well that is an eclipse problem really
drowsy iris
when not being run with J21
last iris
.map(e -> e) I wonder what was there originally and how I could have overlooked that it emptied out for so long ;)
if (newElement != null && ( That thing will be null, there's just the Nullable missing from the other end
steady tiger
so uh i see you have 2 protected fields in ConfigurationScreen
but the class is final
steady tiger
`if (newElement != null && (` That thing will be null, there's just the Nullable...
hmm, can the element supplier be null?
ah yes okay i see
also looking into the size range
last iris
yes, it's a new and optional field
steady tiger
is there a point in it being nullable
instead of say, if it's null just use a 0, max
steady tiger
<a:hmmm:1030417406834774068> so uh i see you have 2 protected fields in Configur...
so about this, is there any reason for that or just an oversight?
last iris
they also could be private, I just didn't want to expose them. give me a sec to check what they are
steady tiger
needsRestart and the container
last iris
I think I have those protected that are used by the other 2 classes
private/protected/package makes no real difference here, other than as marker what's used where
however, needsRestart probably should be public for when people supply a different sectionScreen
last iris
in `switch (needsRestart) {` that `return` is needed otherwise the super call wi...
did you see this one?
steady tiger
yes i fixed it
last iris
steady tiger
also some of the translations can and probably should have fallbacks
i.e. most of these
last iris
fallbacks in which sense?
steady tiger
fallbacks as in a default when the mod doesn't supply one
since most of them are generic
last iris
that's what the translation checker is there. after all, the UI is only active when the mod wants it
steady tiger
well
i don't think mods should be supplying so many generic strings
since they'd end up losing translations
last iris
to try that one out, remove the translations from the jasn and go through the config, then look at the console
steady tiger
as most mods will not have translations for all languages neo will have
Server settings Client settings %s Configuration are all generic
although since you brought it up, i would also have the config screen builtin for mods, obviously overrideable
last iris
those that don't have "section" in their key are from neo having extra sections in each config. without tose, you only get those file-specific ones and the verall title
last iris
let's keep it opt-in for the beginning and put a notice into the changelog that it will be enabled with 1.21.1/1.22
yes, those extra sections are annoying. lucily the MDK doesn't have those, so those mods that expand on that one have a cleaner config
what do you think: remove those extra sections from the neo config and break existing config files (not that neo has much to configure anyway)?
that would massively clean that up
last iris
no, I struggled to find anything to make a name for those in the first place. Modconfigs don't have a name...
but those can easily have one, as they are not handled by the same code as config keys
steady tiger
well they can fallback to type settings and %s Configuration
last iris
those are asTranslationKey(ModConfig modConfig) {
um, they already are below a heading of "config type" already. That's the top-level screen
and when there are multiple configs for the same Type, the default would be the same. the untranslated key at last would give the filename
steady tiger
it's a 2 column layout, there's just nothing there to show it with that weird si...
hmm, i'm trying to make categories be "big" elements as the sound device selection is in the options screen
last iris
yeah, not easy. addBig() is only for config options, not widgets
however, there's one trick: Make the button oversized and the right-side element null
like I do with the headings on the top screen
steady tiger
yeah i noticed that, was just looking at the code
steady tiger
lol
last iris
that#s the correct screenshot
anyway, meet my pets. they have adapted to the new nest fine after a couple of tries moving in and out again multiple times
I only realised that trick today. I'm so used to layouts sizing their contents that I never questioned that
but in mc they only position it, not size it
steady tiger
also, the feedback that a config is saved or that a value was modified isn't really great
i was thinking of either making the done button become Save when there's pending changes
or adding a little icon next to it, or a border
last iris
I was going after what's normal for vanilla, and there the config saves silently
steady tiger
as cursed as this is, I think i like it more over the off centre padded button
last iris
had I noticed that trick earlier, I'd have made it that way, too.
last iris
is that an added section? I would leave them in the right column, otherwise they'd look like section headers.
drowsy iris
Hmmmm, I am not sure I really like that
the click-through buttons
can't we make that tabs?
steady tiger
hmmm
drowsy iris
doesn't vanilla now have that tab widget they use for world creation we can use
steady tiger
nah that's useless
you can nest sections as much as you want
and big configs do nest sections
steady tiger
https://www.curseforge.com/minecraft/mc-mods/configured looking at configured as prior art, it seems to use full buttons too for sections
which from a positioning perspective makes more sense
last iris
yes, and that "Trades" looks like a heading. I could see plenty of people missing it being a button
steady tiger
check the 3rd screenshot
if you give your section a smart name it makes more sense imo
drowsy iris
Okay I have to actually look at the PR myself to judge this properly, it looked like your flow was
last iris
I'd leave it. When those sections are mixed in between values it looks fine as it is
drowsy iris
click on mod -> screen with one button per config type -> then sections
What I meant was that the screen with one button per config type seems superflous when it could be solved with one tab per config type
steady tiger
I'd leave it. When those sections are mixed in between values it looks fine as i...
i left it how it was and added a todo for further discussion then
shartte a mod can have more than one config per type
the tabs will implode at that point
last iris
as cursed as this is, I think i like it more over the off centre padded button
shartte, this is the top-level screen. neo only has one config per type, but there could be any number. from there it goes recursivly with the same screen as sections can be freely mixed in with values
this is a value/section screen with one section at the second position. The button now reads "name...", that picture is old
drowsy iris
shartte, this is the top-level screen. neo only has one config per type, but the...
yes but having to click through that for mods with <= 3 types seems like not ideal UX
anyhow, don't wanna pile requirements on ya
I might take a look myself if I have the time 😅
last iris
if it only has 1 type, the top-level screen doesn't show
but we cannot rally mix the config values from 2 different configs together to avoid a sparse top-level screen
btw, neo's config is a bit sub-optimal as every config has only one section on the top level and nothing below. grrr. we need somethign that has a more typical config for testing
last iris
what?
steady tiger
what do you want lol
drowsy iris
ignore me 😄
steady tiger
also
drowsy iris
that was in response to "but we cannot rally mix the config values from 2 different configs together to avoid a sparse top-level screen"
steady tiger
thoughts on visually tabbing the sections in the title like this?
otherwise you easily get lost
drowsy iris
breadcrumbs good
if you do use this one-screen-per-section approach (if I understand you correctly)
steady tiger
breadcrumbs is the name
drowsy iris
that seems mandatory
steady tiger
right
last iris
that was in response to "but we cannot rally mix the config values from 2 differ...
the point is that we have List<Pair<type, modconfig>> on the top level (1 screen), everything below is List<value | section>
drowsy iris
yes but what stops you from just flatMapping that?
treating the pair as a section, essentially 😛
or rather, inlining its content in the absence of headers I guess
last iris
because I would need to show the values of different modConfigs on one screen
which are saved independently and have different rules when they can be edited or not
also, when people make 2 config files for the same type, they do that to separate the values, us mixing them together again makes no sense
drowsy iris
Yeah if we dont have headers
which given the concept, we don't
I'd have to check how sections are modeled
because I think if you do have a type that only has sections on the top-level
the navigation becomes weirdly deep
last iris
why would you like to flatten all cofnigs and their sections together into one long list?
drowsy iris
with first a very narrow level (Client | Common) and then a wide level beneath
steady tiger
wait what do you want shartte
quick shuttle
How does the config gui looks with Bumblezone. I have a ton of config files, types, and sections
last iris
How does the config gui looks with Bumblezone. I have a ton of config files, typ...
would be nice if you could try it out. only takes one line of code in the mod and using a dev build of neo
steady tiger
(that's bad)
drowsy iris
It's not bad, but eh
steady tiger
yes in the old config pr it had a ton of comments about it being a flawed design
quick shuttle
do you want flattening?
Mojang metadata flattening goes brrr
steady tiger
if you flatten the sections you end up with a HUGE list
drowsy iris
I will try this out for sure, do not take my comments as some strongly opinionated request
no frenky i meant:
client
section 1
section 2
section 3
common
section 1
section 2
section 3
you could merge client+common, but keep nested screens for each subsection
that's what I meant
last iris
that's what we have, just with ModConfigs instead of sections
drowsy iris
hence my comment on "without headers it's meh"
last iris
because the top-level of ModConfigs already is a mix of values and sections
drowsy iris
hence my comment of "if the top-level of a modconfig only contains sections" 😄
in SMP I really don't care though, maty 😄
steady tiger
configurate for instance
last iris
hence my comment of "if the top-level of a modconfig only contains sections" 😄
that's a very special case. neo does it, but everyone following the MDK won't
drowsy iris
I regret wasting your time with these ideas 😓 It's more important to get it out heh
I'll have to port AE2 configs back to ModConfig to try this out, I'll see when I can do that
last iris
yes, getting it out in it's current state and being open to tweak it makes sense. it's not as if tweaking it would introduce an API break
it's literally built to not require any code changes in the configs
(exception: if a config uses lists and wants an "add list element" button. that needs an extra parameter to defineList())
steady tiger
also henry
top level translation keys are a thing
you're hardcoding the section translatables from what i can tell
last iris
top-level? you mean for a whole spec?
last iris
yes, those get used
steady tiger
oh you're trying to use another one for the title
i don't think that makes a lot of sense tbh
it's better for navigation if there's a breadcumb and the names stay the same
last iris
context.modSpec.getLevelTranslationKey(context.makeKeyList(key));
oh, this? Component.translatable(translationChecker.check(parentContext.modId + ".configuration." + key + ".title")));
steady tiger
yeah give me a second
last iris
that could be Component.translatable(translationChecker.check(parentContext.modSpec.getLevelTranslationKey(context.makeKeyList(key) + ".title")));
steady tiger
well not what i meant
the ".title" should just be dropped
the name should stay consistent not to confuse users
last iris
or without the .title ;)
steady tiger
it works better with breadcrumbs, that's why i said give me a second 😛
last iris
I have the title because there's much more space up there than on the button
take all the time you need, I'm not stopping you ;)
steady tiger
i'm too lazy to add a proper test mod with a config and translations
i keep adding stuff to the neo config 
last iris
and btw, initial design goal was to stay as close to vanilla config as possible. that's why I didn't add breadcrumbs, single-value undo buttons and change markers etc
steady tiger
yeah i get it
but vanilla can take some shortcuts
they have at most one deep nesting
and their ui is well known
last iris
i keep adding stuff to the neo config <:LUL:577908625666867200>
I was, too. At least I got to develop the main part of it in my sandbo mod before I moved it into neo
steady tiger
we're dealing with users here trying random configs
last iris
in the end, it's the modders' job to structure their configs in a sane way
steady tiger
where on earth are these "..." coming from
last iris
and if they are not happy with this minimal implementation, they can always use a custom UI or a library mod
steady tiger
steady tiger
where on earth are these "..." coming from
oh you're doing it explicitly
last iris
"..." denotes that a new screen opens instead of a process o direct action. vanilla does this very consitently, that's why I'm enforcing it
steady tiger
thoughts?
i think this looks better
by the end of the day i'll get so annoyed i'll end up writing a proper test mod
fuck it, i'll go do it now
drowsy iris
"..." denotes that a new screen opens instead of a process o direct action. vani...
And I'd agree with it
The ... is a common pattern for that
last iris
the breadcrumbs? sure. I have no aversion against those.
the doubling up of the section name? I dislike doubling that, that's why sections are the only elements that don't have a something in the left column
steady tiger
considering we have the ... suffix
i was about to say, the ... will make the fact that it's not a header clearer
but eh it doesn't really matter
if users complain we can make it big
last iris
indeed. the nice thing is, as I said, that the Ui doesn't need API changes, no matter what we tweak later
but let's go back an hour ;) ... should I remove thsoe extra sections in the neo configs? they don't really serve a purpose and create a weird single-button screen in the UI
steady tiger
sure
i'll reserve my right to make a smol BC to the IConfigScreenFactory API
change the createScreen signature to ModContainer, Screen
so you can do a ::new
having the mc instance doesn't make sense anyway
you can get it from the parent screen
last iris
yes, that would be nice. my first draft had ::new...but that only worked for a single-mod screen... I was quite annoyed when I had to change it ;)
(and now to wait until Eclipse re-indexes 20k files or so after the branch switch...)
I don't think I just broke this? Or did I?
mmmh, i may have...
steady tiger
last iris
yes, I did... Skipped language file: neoforge:lang/en_us.json (com.google.gson.JsonParseException: Error parsing translation for neoforge.configuration.uitext.breadcrumb: Failed to parse either. First: Failed to parse either. First: Not a string: null; Second: Not a json array: null; Second: Not a JSON object: null)
and I don't see the issue... oh, got it. trailing ","
I hate json
btw having those section buttons in the left column does look weird...
steady tiger
steady tiger
btw having those section buttons in the left column does look weird...
see? 😛
last iris
I had them in the right column...
steady tiger
that still looks wrong!
i like the breadcrumbs
although the config list screen also needs to add them
and i'd make the + button full width
i pushed the test config(s)
or didn't
now i did
last iris
I made your golden arrow translatable. I have no idea if RTL languages work in mc, but if they do, they want <
steady tiger
and i'd make the + button full width
thoughts?
last iris
and as a side effect, the translation can now disable breadcrumbs ;)
atm the controls are on one side and the values on the other.
also, the text on that button is veeery short...
if anything, move the button to the right
then it takes exactly the space a new element's content would take
last iris
I had the section buttons on the right, that worked fine and without any headache I can remember.
steady tiger
ah you added an empty string widget
last iris
unless there's a VoidWidget, that's fine in my book ;)
last iris
how do you even launch that test stuff?
steady tiger
you use the test launch config
can we change the label of a config value when it's changed
make it italic
quick shuttle
Make it turn rainbow on random days
last iris
no, because I intentionally do not track the content of single values. that's in the undo manager. all that's available is if it differs from the default
steady tiger
something for a later date™️
hmm, we don't really have something for translating the enum values
last iris
all I get is either a normal client without test mods in debug mode, or a game that doesn't start. can you be a bit more specific, which launch profile I need to use?
steady tiger
steady tiger
not the gametest one
the Tests - Client one
it should work™️
but i don't know if it does actually work in eclipse
if it doesn't, you can run :tests:runClient
steady tiger
ok well i've done all changes
can you add a few more values and maybe another client config to the test
then screenshot them and update the pr desc
last iris
will do. I have something in my sandbox to copy over
btw, this is possible: container.registerConfig(ModConfig.Type.CLIENT, MDKConfig.SPEC, "../../../../../../../config.sys"); and does exactly what it looks like.
you forgot to add a null element after the button... I'll fix it
steady tiger
btw, this is possible: `container.registerConfig(ModConfig.Type.CLIENT, MDKConfi...
not surprising
last iris
and your translatableWithFallback() removed the translationChecker, so modders will never know they can override the default. I'll fix that, too.
