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

Add 1.18 support as well as support for other versions #3

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

DalekCraft2
Copy link

Also, added a translucency tag to blocks.json.

1.14.4 and 1.15.2 do not have completely functional category dumpers yet, but it is fine for now because I doubt that anyone is going to use them soon.

@octylFractal
Copy link
Member

Sorry for the delay in reviewing this. Would it be possible to make this more DRY, e.g. having all the dumpers in a common project, with an interface implemented by each version-specific package? That way we don't need to replace things in a million files if we change them,

@octylFractal octylFractal self-assigned this Jun 8, 2022
@octylFractal octylFractal self-requested a review June 8, 2022 01:00
@octylFractal octylFractal added this to In progress in Octy's All-in-One Task List For EngineHub via automation Jun 8, 2022
@DalekCraft2
Copy link
Author

Sorry for my late response. How should I handle the common project dumpers, since the existing dumpers each rely on imports from different Minecraft versions?

@octylFractal
Copy link
Member

I would add some kind of Identifier-like type, and use that for the RegistryClassDumper/RegistryDumper. Then you can just map to that type in each project. I don't know if it would be worth making the logic for the dumpers also generic by adding more types that act like MC's types.

@DalekCraft2
Copy link
Author

Could I ask for an example of what you are describing so I have a better idea of what to do? What I am understanding so far is that you want for me to make the dumpers from each module implement common interfaces, and to not use generics.

@octylFractal
Copy link
Member

Yes, I'm using the word "generic" here as the non-technical term. I don't really know what example to show that wouldn't just be doing the refactor, since it should be pretty simple? The main point is to just not have a separate RegistryDumper/RegistryClassDumper in every version.

@DalekCraft2
Copy link
Author

But Dumper, RegistryDumper, and RegistryClassDumper all use imports from net.minecraft. How do I separate them without causing errors?

@octylFractal
Copy link
Member

That's why I said

I would add some kind of Identifier-like type

By adding types like what is needed, you don't need the MC type anymore.

Seems like you probably need a Registry interface as well, but you can limit that to what we use, and implement it in each version by delegating to that version's Registry.

@DalekCraft2
Copy link
Author

So I would create interfaces in the common module, and create classes in the version-specific modules which implement the interfaces and delegate the methods to the net.minecraft classes?

@octylFractal
Copy link
Member

It's not strictly necessary for Identifier, since it's really quite simple (you could even copy ResourceLocation from WorldEdit), but yes that is the idea.

@DalekCraft2
Copy link
Author

Things have become complicated now as I realize that Registry is an abstract class and is extended several times, so I can not easily delegate to it by extending it unless I was to make delegates for each subclass, and, even then, I am unsure of how I would convert the native Minecraft class instances into delegate instances.

@octylFractal
Copy link
Member

I think you're making this too complicated -- we only need a small bit of the API that Registry provides, we don't need to clone its hierarchy. In fact, taking a closer look, we only use registry.get(v), and immediately pass it to getProperties -- so there's no need to even have a Registry type, you could just modify public abstract List<Map<String, Object>> getProperties(Identifier resourceLocation, V object); to instead be public abstract List<Map<String, Object>> getProperties(Identifier resourceLocation); (with the non-MC Identifier, of course), and do the registry lookup in the implementation.

@DalekCraft2
Copy link
Author

DalekCraft2 commented Jul 3, 2023

We also use Registry#getIds() in that same line. What should be done about that?

Oh, wait, I can just replace getRegistry() with getIds().

And I can remove the Identifier delegate by returning the IDs as Strings instead of as delegates.

@DalekCraft2 DalekCraft2 marked this pull request as draft July 3, 2023 17:48
@DalekCraft2
Copy link
Author

DalekCraft2 commented Jul 3, 2023

I have finished what you asked, though I still need to update the 1.18 module from 1.18.1 to 1.18.2, and I need to get the 1.14.4 and 1.15.2 modules' category dumpers to work.

I need to merge the latest commit on the master branch into it, too.

@DalekCraft2
Copy link
Author

DalekCraft2 commented Jul 18, 2023

I have now noticed that the newer commits of this repository have removed the category dumpers. May I ask about why that was done, and should I remove them from the older ones as well?

Update: I figured out that they were actually consolidated with their respective -TypesDumper classes, but I am keeping them as separate classes for now until I figure out how to make the single-class system work with the core module. In other words, I need to make RegistryClassDumper from the core module use the code from this repository's current revision of RegistryClassDumper without using Minecraft imports at all.

... although the 1.18.2 module can't run properly right now. I commented out the category dumpers in 1.18.2 because it seems that the newest commit for this repository has removed them entirely, and I am waiting for an answer as to whether I should remove them from all versions.
I also changed some `System.err` usages to `System.out` because it was more appropriate, and updated some plugins.
May as well do this to all of them.
I am not too experienced with Gradle, so feel free to change how I did this.
@DalekCraft2 DalekCraft2 marked this pull request as ready for review August 29, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants