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

Mapper backend refactor and texture deco improvements #280

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

Conversation

brachy84
Copy link
Member

@brachy84 brachy84 commented Dec 8, 2024

  • add an AbstractObjectMapper<T> class
  • mappers can now be created using the builder or by extending the new class
  • texture decos should now be thread safe (i dont really now what im doing there)
  • some code structure improvements for texture decos
  • some improvements for texture decos to avoid creating them multiple times
  • this branch was successfully tested for backwards compat with gt

@brachy84 brachy84 added bug Something isn't working refactor Something is rewritten lang-server Issues and PRs relating to the Language Server labels Dec 8, 2024
Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

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

this was an issue prior to this PR, but what should be done about animated textures, if anything?


public boolean hasTextureBinder() {
if (this.hasTextureBinder == null) {
for (Method method : getClass().getDeclaredMethods()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does this need reflection?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a default implementation to check if the method is overriden. Only then the texture binder can be defined


@Override
public void provideCompletion(int index, Completions items) {
if (index == 0) items.addAllOfRegistry(ForgeRegistries.BLOCKS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to have completion for properties of blockstates?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but we would need information about what the first param currently is


@Override
public void provideCompletion(int index, Completions items) {
if (index == 0) items.addAllOfRegistry(ForgeRegistries.ITEMS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible for the valid metadata of items to have autocompletion? pretty sure its a "no"

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible, but id argue that its not worth it


public class BlockStateMapper extends AbstractObjectMapper<IBlockState> {

public static final BlockStateMapper INSTANCE = new BlockStateMapper("blockstate", null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is the only place you are going to init BlockStateMapper, why pass null here and not drop and argument and call super(name, null, IBlockState.class)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was doing that for item so that mods can extend that class for a template (for example gregtech metaitems). I guess its not very useful for blockstate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lang-server Issues and PRs relating to the Language Server refactor Something is rewritten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants