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

Mod compat #18

Merged
merged 27 commits into from
Mar 18, 2022
Merged

Mod compat #18

merged 27 commits into from
Mar 18, 2022

Conversation

Qendolin
Copy link
Contributor

@Qendolin Qendolin commented Feb 5, 2022

@Qendolin Qendolin marked this pull request as ready for review February 5, 2022 14:14
@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 5, 2022

Test World.zip

@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 5, 2022

I still need to do one last test, which is creating a dummy mod and adding ConnectibleChains compatibility to it

@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 5, 2022

It works! Ready for review.

java/net/fabricmc/example/ExampleMod.java

if(FabricLoader.getInstance().isModLoaded("connectiblechains")) {
  ChainTypesRegistry.register(Items.DIRT);
}

resources/assets/minecraft/models/entity/chain/dirt.json

{
  "textures": {
    "chain": "minecraft:textures/block/dirt",
    "knot": "minecraft:textures/block/dirt"
  }
}

image

@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 6, 2022

There might be an issue with resource priority where f.e. valley craft cannot override the default copper_chain model because it has a lesser priority. I haven't encountered this issue in my testing but I don't see why it cannot happen. I'll have to do some changes to how builtin types are loaded.

@Arathain
Copy link

Using structure blocks to move chains has weird side effects - seemingly only chains which share the same X axis are stored properly. Screenshots attached below.
2022-02-23_12 07 03
2022-02-23_12 07 16

@Arathain
Copy link

removing this line of code seems to fix things

@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 23, 2022

That is very interesting, since I already had that exact same issue and fixed it. But now it's back.
Removing that line only works when no mirror is applied.

@Qendolin
Copy link
Contributor Author

Qendolin commented Feb 23, 2022

image
I've tested all possible combinations of mirroring and rotation, this should work now.

Copy link
Owner

@legoatoom legoatoom left a comment

Choose a reason for hiding this comment

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

Amazing work. Just a small tweaks in regards for error handling.

  • Do not throw errors, log them.
  • No longer use lo4j as there are now other ones that fabric wants you to use. (look in discord I do not know the exact name right know)

Finally, I am seeing a lot of classes that do not have an explanation for their existence as javadoc. It is just very important to me that I can read your code later.

@Qendolin
Copy link
Contributor Author

"No longer use log4j as there are now other ones that fabric wants you to use.". Do you mean slf4j? I'm not sure if that is available before 1.18.2

@Qendolin Qendolin requested a review from legoatoom March 17, 2022 17:41
Copy link
Owner

@legoatoom legoatoom left a comment

Choose a reason for hiding this comment

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

Nice

@legoatoom
Copy link
Owner

legoatoom commented Mar 18, 2022

"No longer use log4j as there are now other ones that fabric wants you to use.". Do you mean slf4j? I'm not sure if that is available before 1.18.2

Does this mean that the current code does not work in 1.18.2?
Or can I publish this new version?

@legoatoom legoatoom merged commit a1e899e into legoatoom:master Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants