Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recipe/Condition Factories cannot be registered from code #4274

Closed
DaedalusGame opened this issue Aug 2, 2017 · 3 comments
Closed

Recipe/Condition Factories cannot be registered from code #4274

DaedalusGame opened this issue Aug 2, 2017 · 3 comments

Comments

@DaedalusGame
Copy link

The only way to register recipe and condition factories is currently to insert a FQCN into a random json file which seems put in as an afterthought. Code-related identifiers have no place in data files, especially not fully-qualified classnames. Not only does it mean that plugins that statically analyse code for unused parts have a harder time and need to be needlessly complex to even work, but there's also no point to having a data file tell the engine which factory to load. Factories need not be loaded based on user-specified conditions, and they need not be loaded more than once.

Hence Recipe and Condition factory registration should be moved into an event on startup or similar, to cleanly seperate code from data.

@JamiesWhiteShirt
Copy link
Contributor

JamiesWhiteShirt commented Aug 2, 2017

Absolutely agreed. I see no benefit from registering these through JSON in the first place. What you lose is static verifiability and the ability to construct these objects however you want. What Forge gained is complexity. You could say it is equivalent to registering blocks and items through JSON.

You could question why the built-in factories are not added via JSON.

@LexManos
Copy link
Member

LexManos commented Aug 2, 2017

It's a code style choice, and a functional choice, which was NOT an afterthought.
I decided to go this route to allow things to easily indexable without having to search through binary class files by resource pack makers and other recipe related tools. Having one central place to see what is available to them is important. It would be nice if you didn't have to use fully qualified names, or you were forced to use some form of documentation on what each factory did/was used. But that type of stuff can't be forced code wise so here we are.

It's also designed to force modders to think about their recipes and clearly define how they are structured and hopefully make them generic and reusable. As well as to remove any conditional definitions of factories because of what you feel like that day. As having a stable target on the pack dev side is important. On top of that i'm honestly thinking of maybe expanding the factories to be javascript instead of just json. So that the actual functionality is controlled in the file and would potentially be more self-documenting as people could inspect what's actually going on. But that moves things to an entirely different realm which I don't want to do at this point.

Again, at the end of the day this comes down to code style. Yes, having to put full class names into a json file is a bit annoying. But it's a fairly standard thing in plugin systems to define your entry points somewhere outside of code. I have also toyed with the idea of simply specifying a single class, Or forcing you to have a single class named something specific, that would allow for multiple factories to be defined in one .java file. But I haven't sat down and worked through those ideas yet.

Part of the point is that I have to think of more than just people writing mods when it comes to this. I have to think of pack makers, end users, and 3rd party tools. Because yes those types of people are a big part of the recipe community/audience. You guys should care about them. Having a central place with this information that doesn't need to be parsed out/decompiled from binary classes is important. Toss in a few comments {Which reminds me, I think I turned comments on if not I need to fix that} in those json files to document how to use the factories and you have a nice resource that non-programmers can reference!

"You could say it is equivalent to registering blocks and items through JSON." Which, if you haven't been paying attention to the last 2 years of development, is exactly where Minecraft is trying to go. Deal with it.

"You could question why the built-in factories are not added via JSON." This one is honestly because at the time I was developing the entire loading system I had to re-write that stuff like 5 times and I wanted to move on to other things. One of them was the concept of registering this stuff in code. Which is why the register functions still exist. But I abandoned that in favor of giving more power to the pack makers. I should go back and clean that up into a _factories.json but that's later.

TLDR: Yes I know specifying full class names is a little dirty/annoying to people. I have good reasons for going this route. And I am open to ways to make it cleaner without sacrificing the ability for non-programmers to know/handle/use this data. But for now this is what we got.

@LexManos LexManos closed this as completed Aug 2, 2017
@JamiesWhiteShirt
Copy link
Contributor

Even though I don't think the outcome is satisfactory, it's nice to get some arguments for why the system is the way it is. Better than "Use the JSONs."

Keep it up 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants