TheElectronWill / night-config

Powerful java configuration library for toml, yaml, hocon, json and in-memory configurations. Serialization/deserialization framework.
GNU Lesser General Public License v3.0
242 stars 28 forks source link

Night Config Integrated fork Question #140

Closed NeusFear closed 1 year ago

NeusFear commented 1 year ago

Hello,

I apologize if this is the wrong place to ask this, but I am working on a java game engine and night-config is almost exactly what I need as far as functionality goes, but there are significant differences to how users of my engine need to interact with a config file compared to the way it works as default in night config.

I have read about the LGPL license and believe that what I want to do is allowed but I wanted to run it past you first.

I intend to have a TOML config parser in my engine and the way night config's parsers work is great, but I want to design a completely different interface which is more inline with what users of my engine would be used to, so is it allowable for me to essentially pull your code from night config core and toml which meets my needs and redesign the interface in my "greater work" as lgpl puts it?

My repo is currently unlicensed as it's in early development, but I have considered LGPL for it as well, so that requirement should be met. Anything else that would be needed for this to be allowable and fair?

Thanks!

TheElectronWill commented 1 year ago

Hello,

Thank you for asking the question!

is it allowable for me to essentially pull your code from night config core and toml which meets my needs and redesign the interface in my "greater work" as lgpl puts it?

With LGPL you have two options:

  1. Use night-config as dependency of your project, without modifying it (the LGPL reads something like "use the licensed work (night-config) through interfaces provided by the licensed work (night-config's api)"). If you do this, you can freely choose the license of your project.
  2. Take the source code of night-config, modify it and redistribute it. In that case, your modifications must be redistributed under the LGPL or GPLv3, and comply with additional requirements of the LGPL license.

I believe that option 1 would be easier for you: add night-config as a dependency, then provide an interface on top of it for the users of your engine. This is what Minecraft Forge does, for instance. There's no need to maintain a fork in that case.

NeusFear commented 1 year ago

That all makes sense, thanks for the timely reply!

NeusFear commented 1 year ago

Hello, I am reopening this issue because using the route you reccomended I would need to overload a couple constructors which are not public. Was wondering if this is something you'd be willing to widen, OR have a better alternative to my approach.

To give better context I'll explain why I am even doing this: (Feel free to skip if you don't care) Essentially my game engine has a sort of "virtual file system" which can provide "files" from many sources, be that the engine, game, a mod, etc. SO, I need to basically Overwrite the #defaultResource method to pull from my vfs rather than only the current resources since for example mods which provide configs should be initialized from the game's mod loader and thus need to be retrieved through the VFS.

Now onto the stuff that I need so far (I think)

I suspect there will be more things which I need to overload but it becomes difficult to keep writing code without being able to incrementally test it. I attempted to create a fork of the repo and add it as a submodule to test changes for a future PR, however sumobules can be janky, and I was not able to get the separate projects (eg. toml, yaml, etc.) to differentiate themselves in my project.

Any insight or alternate solutions welcome, thanks for your time!

TheElectronWill commented 1 year ago

Are you using java.nio.file.Path for your "vfs"? It can represent a resource that is an actual file on the disk, a file inside a jar, or anything else. NightConfig works with Paths, for example with defaultData (instead of defaultResource).

Second question: how do you expect games or mods to use your engine in order to ensure that they get the right paths? It may be easier to delegate the construction of a configuration to the engine and pass the resulting Config or FileConfig to them. Presumably, every mod will follow the same structure, so the engine knows that.

At least, the engine could provide utility methods to get the correct paths, like:

public class PathFactory {
  public statis Path engineDataPath(String path) {
    // resolve relative to the engine's data directory
  }
  public static Path modResourcePath(String path) {
    // resolve relative to the mod's resource directory or jar or whatever
  }
  // ...
}

Now, I could open the constructors of some classes, but that would expose some implementation details and prevent me from changing them in the future without breaking the API, so I would prefer not to.

Instead of extending those classes, how about providing your own classes on top of it, using composition instead of inheritance? For instance, if you need a special X, you can do:

public class TunedX {
  private final X x;

  public TunedX(X underlying) {
    this.x = underlying;
  }

  public void method() {
    super.x.method();
    somethingElse();
  }
}

If X is a Config (or FileConfig, or another subclass of Config), you can extend ConfigWrapper to help you do this.

NeusFear commented 1 year ago

I can get the file as an InputStream pretty easily, and technically as a path, but they're inconsistent across sources so I have not got a simple way to expose that. Is it possible to just return a Config instance just simply by parsing an InputStream?

NeusFear commented 1 year ago

A defaultData which accepts an "in memory" config or a String of the contents of a config etc would make this very trivial actually. Then I could do something along the lines of

    public static FileConfig getOrCreateFileConfig(GameFileSystem fileSystem, Identifier resourceIdentifier) {
        boolean exists = Files.exists(Paths.get(getConfigFileLocation(fileSystem, resourceIdentifier)));

        if (!exists) {
            FileConfigBuilder builder = FileConfig
                    .builder(getConfigFileLocation(fileSystem, resourceIdentifier))
                    .defaultData(createFromFileSystem(fileSystem, resourceIdentifier))
                    .build();
        }
    }

    public static String getConfigFileLocation(GameFileSystem fileSystem, Identifier resourceIdentifier) {
        return fileSystem.getConfigsDirectory() + "/" + resourceIdentifier.getNamespace() + "/" + resourceIdentifier.getName() + ".toml";
    }

    public static Config createFromFileSystem(GameFileSystem fileSystem, Identifier resourceIdentifier) {
        ConfigFormat<CommentedConfig> configFormat = TomlFormat.instance();
        ConfigParser<CommentedConfig> configParser = configFormat.createParser();
        try {
            return configParser.parse(fileSystem.getResource(ResourceType.DEFAULT_CONFIG, resourceIdentifier).openStream());
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }
NeusFear commented 1 year ago

It seems like you even had a FileNotFoundAction#copyData method that accepts an input stream, it just needs exposed in a defaultData method, and I could pass that right along which would be even simpler.

TheElectronWill commented 1 year ago

It seems like you even had a FileNotFoundAction#copyData method that accepts an input stream, it just needs exposed in a defaultData method, and I could pass that right along which would be even simpler.

You can already use it:

builder.onFileNotFound(FileNotFoundAction.copyData(myStream));

:)

TheElectronWill commented 1 year ago

But if you open an InputStream from a File, I see no reason not to pass the File directly to the builder. It's going to do the same work but automatically. You build the path at runtime anyway, so you can change it how you see fit.

NeusFear commented 1 year ago

I did not realize I could do that already, (facepalm) next I get to my PC I'll try that out. As for passing the file, I cannot guarantee through my VFS that the resource is a file, it could be any number of sources.

NeusFear commented 1 year ago

So I have tried the above suggestion and it seems like it should work, and it creates the file, however it is empty. I don't suspect that it's an issue with my InputStream because I use it in my own .asString method which prints just fine. I tried creating the file and saving it with the included test code. I also included in the third block a snippet where I tested to make sure that it just was not writing properly and attempted just printing the contents of the in-memory config, but it also comes up empty. See below code:

Test code to create and copy data:

        FileConfig fileConfig = FileConfig
                .builder(getConfigFileLocation(fileSystem, resourceIdentifier) + "/" + resourceIdentifier.getName())
                .onFileNotFound(FileNotFoundAction.copyData(Objects.requireNonNull(createFromFileSystem(fileSystem, resourceIdentifier))))
                .sync()
                .build();
        fileConfig.save();
        fileConfig.close();

Accompanying code which tests that my input stream is good, and copies the stream into the above code block:

    private static InputStream createFromFileSystem(GameFileSystem fileSystem, Identifier resourceIdentifier) {

        //This prints accurately to the console the contents of the config file
        Log.info("pre: " + fileSystem.getResource(ResourceType.DEFAULT_CONFIG, resourceIdentifier).asString());

        try {
            return fileSystem.getResource(ResourceType.DEFAULT_CONFIG, resourceIdentifier).openStream();
        } catch (IOException e) {
            Log.crash("Could not read config file InputStream: " + resourceIdentifier, new RuntimeException(e));
        }
        return null;
    }

Finally the code which tests that the in-memory config which should be created including the contents of the file:

        FileConfig config = TVConfig.getOrCreateFileConfig(ClientBase.getInstance().getFileSystem(), new Identifier("game", "test.toml"));
        ConfigFormat<CommentedConfig> tomlFormat = TomlFormat.instance();
        ConfigWriter tomlWriter = tomlFormat.createWriter();
        String toml = tomlWriter.writeToString(config);
        Log.info(toml);

Does it look like there is something wrong with what I'm doing here?

NeusFear commented 1 year ago

Ah yes, a missing fileConfig.load();

I think everything is working as intended now, thank you so much for your help! Glad to see that you thought of quite a few things in the design on this library, nice job!