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

Minecraft 1.14 #129

Open
wants to merge 28 commits into
base: 1.12
Choose a base branch
from
Open

Minecraft 1.14 #129

wants to merge 28 commits into from

Conversation

maxanier
Copy link

@maxanier maxanier commented Mar 22, 2020

My mod Vampirism is using Guide-API to provide a guide book since Minecraft 1.7.10
Since Vampirism is now available for 1.14 and 1.15, I need a Guide Book for those MC versions as well.
There already is Patchouli, but it has some limitations and more importantly the content would have to be ported. Therefore I decided to update Guide-API to 1.14 (and later 1.15) since you generously published this under MIT licence.

There is still quite some things to do (recipes, book/item registration, debugging), but it already compiles. I am mainly opening this WIP PR to let you know I am working on this and to discuss a few things:

I am not sure how involved your are in MC modding right now and if you are even interested in MC1.14+, so I think there are several options once/if I got a properly update version.

a) you merge this PR and continue maintaining the mod for 1.14+ yourself
b) you add me to the curse project and I can publish new versions for MC1.14+
c) I create a new curse project and add you as an (old) author
d) I include/shade Guide-API in Vampirism.

I'm fine with all options. Guide-API is a cool mod and I just would like to continue using it :)

@TehNut
Copy link
Member

TehNut commented Mar 25, 2020

Looks pretty good so far, but I have a few overall notes:

  1. I do not like Forge's config system, especially if it requires removing the per-book spawn setting. I would much rather use a JSON config.
  2. You removed the namespace from the API package which is, well, bad.
  3. I hate TOML. No TOML allowed. >:( For the mod metadata, you can copy this Gradle script and convert the file to JSON like this.
  4. I assume it's just an artifact of the porting process, but there's inconsistent whitespacing throughout the changes.

@maxanier
Copy link
Author

maxanier commented Mar 28, 2020

Thank you for your feedback.

  1. It has its pros and cons. I am planning to reintroduce the per-book spawn setting. It is possible with the underlying nightconfig system.
    I would much rather just copy the old forge config code, but that won't work due to licensing issues.
  2. Yeah, my bad. I tried separating things in different source sets (which did not work since the api depends on non api code) and messed up when reverting it.
  3. I tried that, but did not manage to get it to work in intellij. I need the .toml file in my "out" directory.
    Gave up after 20min. If you really want it you can migrate it afterwards (that file is modified only every few months, so I think a tiny bit of toml should be acceptable)
  4. I will run a formatter once I am done.

My TODO list:

  • fix book item model
  • fix translations
  • test with an actual book (Vampirism's book)

@TehNut
Copy link
Member

TehNut commented Mar 28, 2020

which did not work since the api depends on non api code

That's something I've been wanting to fix, but never could without breaking mods. This could be a good time to take care of that.

I tried that, but did not manage to get it to work in intellij.

Hm, I use IntelliJ and haven't had problems with it. You did make sure to apply it, right?

@maxanier
Copy link
Author

Ok, I will have another look if its reasonable to separate the api properly.

Yes I did apply it. Gradle is always fun, especially with all kinds of (build) caches. I made sure they build.gradle matched the HWYLA one, but it still did not work

Still prints a missing model warning
I am not sure what this was required for, but both unicode and non unicode seems to be working fine in 1.14
@maxanier
Copy link
Author

maxanier commented Apr 3, 2020

One thing that is causing some trouble, is that the language data is (for some reason) only available during registry events and after the LoadComplete event. So it is not possible to localize text during any of the events suitable for building the book content/categories. (I don't want to construct the book content during the item registry event).

Most content could be localized when displayed, but e.g. PageHelper.pagesForLongText requires the full translation to be able to choose the appropriate number of pages.

I might have a look at delaying the book content initialization until the book is actually used for the first time. This might create a little bit of one time lag though. I will also have a look if we can avoid loading the book content on server side.

@maxanier
Copy link
Author

maxanier commented Apr 3, 2020

Seems to work fine so far.

What do you think about increasing the book width a little bit. Yes, it would look different than vanilla books, but I think it would greatly improve reading comfort.

About splitting of an api package/module:
After thinking about it some more, I don't think it makes a lot of sense. Firstly, endering and content are way too interweaved and a lot of interfaces would be required.
Secondly most mods would probably depend on the full jar anyway. The main benefit of GuideAPI over alternatives like Patchouli is that it is very simple to create custom pages or rendering, and for that the core classes of GuideAPI are necessary.
Lastly, I don't want to spend way too much time on this :)

@TehNut
Copy link
Member

TehNut commented Apr 3, 2020

Lastly, I don't want to spend way too much time on this :)

Sounds like my reason, so it's good enough of a reason for me!

@maxanier
Copy link
Author

maxanier commented Apr 8, 2020

So, I think this is ready.
I have also updated Vampirism's guide book and everything appears to be working properly.

Page width

I changed the width of the book, it is now more a square shape. I think this makes guide books much easier to read than before, since more than two words fit in one line. Pages generally don't look as crammed. This means that updating mods might have to adjust their content, if they had manual line breaks or similar. If you don't like this I can revert the commit, but I generally think this makes GuideAPI more competitive with Patchouli

Configuration

The configuration, which now supports the perBook setting, is still in toml. I understand that to hat toml, but I don't think that is reason enough to force players to deal with different config file formats for mod configs. It is good that all mod configs are in the same format and Forge just decided that this format is now toml (not sure why, but probably because the new library does not support the old). It is a tiny config file, so it shouldn't really matter which format this is in.
If you still absolutely don't want to use toml, we could use either yaml or hocon and ship the respective library extension with Guide-API (not sure what happens if Forge updates the used base library version)

Formatting

In the last commit I ran the default Intellij formatter over the entire codebase. This could also be reverted.
You can browse the changes excluding the formatting here:
https://github.com/TeamAmeriFrance/Guide-API/pull/129/files/7829beec83e4e0d6b0984a192ef98a7fe14c68b6

@maxanier maxanier marked this pull request as ready for review April 8, 2020 21:49
@maxanier
Copy link
Author

@TehNut Did you already have time to look at this?
I am quite occupied myself right now, but if only small changes are requested I can promptly implement them and we can get Guide-API for 1.14 going soon

@TehNut
Copy link
Member

TehNut commented Apr 23, 2020

I'll try and make time to look through it tomorrow.

Copy link
Member

@TehNut TehNut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some trivial things I'll take care of, otherwise this looks fine.

Comment on lines 41 to 42
MinecraftForge.EVENT_BUS.addListener(this::onServerStarting);
FMLJavaModLoadingContext.get().getModEventBus().addListener(this::onServerStarting);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just forgot to remove it

@maxanier
Copy link
Author

maxanier commented May 3, 2020

So, since I really need a 1.14 version (users are asking for Vampirism guide book and asking a lot of questions on CurseForge and Discord which would be answered by the book) and it didn't seem like you are very committed to MC modding right now, I decided to release my fork on Curse on my own.
It will be appear here once approved: https://www.curseforge.com/minecraft/mc-mods/guide-api-village-and-pillage
I have added you as former author (with a 50:50 split, I can add tombenpotter too if you want)

It has a different modid: "guideapi-vp" and a different package. So, if you should decide to continue actively working on and supporting GuideAPI, there shouldn't be any conflicts and Vampirism could migrate back.

I will start working on 1.15 soon.

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

Successfully merging this pull request may close these issues.

2 participants