Major Refactoring of Bootstrap Code

[shartte]

So, this is not 100% finished but should be servicable as the basis for discussion for #70.

Highlights:

• Default state mapper is gone, FORWARD/UP are now unlisted properties instead.
• IAEFeature and all block/item base-class methods/interfaces that would just set bootstrapping properties have been removed. Same for all feature handlers.
• Instead, the bootstrapping code in ApiBlocks, ApiItems, et. al. now does the full bootstrapping of blocks and items. If rendering customization is needed, it can be outsourced into a class and placed next to the items/blocks in question. This allows for further removal of custom base classes (i.e. stairs) and vanilla classes can be used instead if we desire to do so. This also means that the association between a block and features (for toggling them on/off) is centralized in ApiBlocks, etc. and the feature toggle automatically applies to all rendering initialization as well. Added benefit: The Block class isn't even constructed when the feature is turned off. In some cases, the entities associated with a block were registered even if the feature was toggled off.
• Block and tile entity setup have been consolidated to reduce code duplication. (See BlockDefinitionBuilder / ItemDefinitionBuilder)
• Lots of item JSON models can now be removed again since the default block state is now used to select an item model automatically if no override is specified during bootstrapping.

TODOs: Sky Chest used an item TESR. Since that feature will be removed from Forge, it should move to a separate item JSON model instead.

p.s.: If you have any in-depth questions, just shoot

pps: Please note that this is a PR for the restructuring branch

[Elix-x]

TODOs: Sky Chest used an item TESR. Since that feature will be removed from Forge, it should move to a separate item JSON model instead.

They said in 1.7.10, that they are gonna remove this in 1.8. They did remove it... And recived 5 PRs adding it back. They added it back with removal in 1.9... Then in 1.9.4... Then in 1.10... It's still here :laughing: ...

[Elix-x]

@yueh you can look at files, if there are too many, i'll merge and we can look directly in the branch.

[yueh]

Probably takes a while to review it.

As heads up, It is a good practive to squash PRs. Having commits floating around like Oops. Forgotten file or Fixup is really bad in the long term. Should we ever have to revert a PR for some reason and github might fail, you do not want to search for multiple related commits.

Also master should be able to compile/build at any time/commit. If you screw up, ammend. That is the awesome part about git that you can rewrite history before contributing/merging. While afterwards a rebase on master can be quite tricky.

Another good pratice is to create an issue for every single bugfix and reference it in the commit message. It makes it so much easier to create a changelog like http://ae-mod.info/docs/rv3/release-notes/ from the commit history. While you can reference PRs in a similar way, you cannot predict the PR# before creating the actual PR, thus an issue is more reliable.

[Elix-x]

@yueh i branched off main port branch into separate branch (PR is targetted there), so we can experiment without damaging valuable code, if corruption happens.

[shartte]

As heads up, It is a good practive to squash PRs.

I almost always do that for bugfix or enhancement PRs. I'll tyr to keep further commits in this branch as clean as possible and try to separate them into logical units.

[Elix-x]

Edit: https://github.com/AEModernMCPort/Applied-Energistics-2/issues/70#issuecomment-241279484.

[shartte]

Did you go through all of it already? What can I work on here to make this mergeable?

[yueh]

I do not see anything being really off.

Probably a few smaller things like FeatureFactory could be an interface But that is nitpicking. Or I have not really made up my mind regarding something like return is -> { 200 LoC }.

The only thing which should probably be reconsidered, but is not related, is the energy cell. Why does every state have its own model and not just one of these extended forge models and swap the texture? That probably applies to a couple of other items or blocks.

[shartte]

I thought about FeatureFactory, but have still not made up my mind about the interaction between Registration, FeatureFactory, and ApiBlocks. I think we first need to decide whether we keep how the API is currently exposed to Plugins or not. If we decide to do anything differently, I'd do that in a subsequent change.

Regarding the long lambdas: Agreed, should go with method references in those cases. Energy Cell: We can clean that up, but that was not introduced in this PR, so I would do it subsequently.

[yueh]

No idea about the API, too. But mostly about the external part. Like should we keep a static instance() or inject it into any mod annotated with @AEAddOn and let them deal with it. There might be even some issues as AE disables certain items after the registration, when they are already present like silicon. But honestly I have no idea, if there might be issues. It is probably nothing anyone ever used.

Clearly offtopic, but I have a particular large backlog of what would still be needed in general for AE. But I am not really convinced about how to handle it in terms of project management. Like adding all issues here is not really the option once we merge it back and could loss that information or at least be in an inconvenient location

[shartte]

That's a really good question regarding project management. I still think issues are the right way to go, but if it's post-port work, I'd put it into the main repo.

Regarding the API: I checked out what JEI does and it does seem elegant. They use the FML provided ASMTable to find all classes in all all mods that are annotatd with @JEIPlugin and then instantiate them, and pass the JEI API instance to the newly created instance.

Right now the biggest issue with the static instantiation is the fact that when the API is instantiated, it creates the block/item definitions, and that currently needs access to the FeatureFactory to build the bootstrapping tasks. This also immediately breaks once something outside of AE accesses the API before we do pre-init, because the config is not loaded at that point.

Removing the static access to the API and instead opting to expose it to plugins once we have explicitly initialized it would solve this. But if possible, I'd like to separate something like that from this PR and discuss it separately, since it does have bigger implications. AE itself uses the static API entrypoint extensively and that would also need to be changed.

[yueh]

Regarding the API: I checked out what JEI does and it does seem elegant. They use the FML provided ASMTable to find all classes in all all mods that are annotatd with @JEIPlugin and then instantiate them, and pass the JEI API instance to the newly created instance.

The instantiation part might be a bit tricky due to how broken the class loading with forge can be. It works for JEI because it a special plugin and not a normal mod. Also the object can be thrown away afterwards as it was only used to register recipes (it more similar to our own registration process). We probably need something like @AEAddOn to indicate a mod is an AE AddOn and require them to implement something like an AEApiConsumer, who gets passed the API after it is constructed. As the addon needs to keep a reference to it, we cannot really construct that class and leave it to the GC and hope the addon dev somehow ensure that the reference is not lost.

The general process should probably be

1. Load config
2. Bootstrap API according to the config
3. Announce API publicly
4. normal pre/init/post part

Having the blocks/items as part of the API also allows addons to rely on them being there, without having to resort to some loading order conflicts/magic when a mod needs to be loaded before AE for whatever reason but still depend on some AE2 parts.

The only issue I would see with is, is the removal of duplicate items. If I am not completely wrong, it is done during post init.

project management

Issues are clearly the way to go. Just the location of them is tricky. Even post port we need to be able to reference old ones easily. The might contain essential information about why we choose one way over another. If the question rises again, we can just point them there. If course there are other ways like using the github wiki to maintain important information, but not the discussion about it. In a similar way nothing should be fixed, what is not reported as an issue. Some random reddit post, or irc discussion is simply not ideal for it. Whoever wants a something to be fixed or added should make a github issue about it. No special treatment just because some random modpack maker thinks they are above that.

[Elix-x]

We can create a "important post-port label" and mark all important issues and PRs with it. Also use correct self explanatory names for these issues.

[shartte]

@elix-x Can you merge this, or do you want to review it further first?

[Elix-x]

Api followed in #87.