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

chore: remove usage of RegisterCopyStrategy #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pollend
Copy link
Member

@pollend pollend commented Mar 28, 2021

I was looking at the usage and I don't see any kind of registration in module space. not sure what this serves at the moment to have things registered in this fashion. At the moment we can bind things the same way we bind things such as TypeHandlers and they are not driven by scanning for classes. If there is no immediate use for this in module space I propose dropping this.

If we are looking for more generic solutions I think this could be solved with (MovingBlocks/gestalt#87). not sure how to handle the generics thought. there is some existing mechanism to look at generics just not a way to look at it from the definition. maybe something to investigate down the line @DarkWeird .

ref Commit: MovingBlocks/Terasology@f9ce52f

MovingBlocks/Terasology#4593

@DarkWeird
Copy link
Contributor

This is using at engine itself. :)
You can remember drop bug at your gestaltv7 pr. It is was not registered Vector3fCopyStrategy(because nui was package module, but reflection wasn't at package)

@keturn
Copy link
Member

keturn commented Mar 28, 2021

Recapping my thoughts after that brief discussion:

  • I don't feel the need to remove the @RegisterCopyStrategy annotation on principle. Putting an annotation on your implementation class is easier than hunting down every method that populates a CopyStrategyLibrary.
  • That said, I don't understand why nui-reflect defines strategies for JOML classes in its copy.strategy package. I couldn't find a way that it's ever using them? I believe they might be used for AutoConfig stuff because that's supposed to work for more than just primitive properties, but I failed to connect the dots.

@pollend
Copy link
Member Author

pollend commented Mar 28, 2021

Recapping my thoughts after that brief discussion:

* I don't feel the need to remove the `@RegisterCopyStrategy` annotation on principle. Putting an annotation on your implementation class is easier than hunting down every method that populates a CopyStrategyLibrary.

* That said, I don't understand why nui-reflect defines strategies for JOML classes in its `copy.strategy` package. I couldn't find a way that it's ever using them? I believe they _might_ be used for AutoConfig stuff because that's supposed to work for more than just primitive properties, but I failed to connect the dots.

I looked through module space and these are the only instances these annotations are used for. more inclined to drop them so there is no pretense that the annotation is used anywhere. more confusing to have them then not have them. If were not using them for registration then what do they serve :?. just as easy to add them back in if we need to have some kind of registration process for copying properties in module space but at the moment just make things complicated.

@keturn keturn added this to the v3.0.0 milestone Apr 1, 2021
@keturn
Copy link
Member

keturn commented Apr 2, 2021

I didn't feel like switching to this branch to get the Vector4i strategy, so I did this instead: MovingBlocks/Terasology@9afe704

but that really doesn't solve the question of whether to keep the annotation or which of our various several packages these CopyStrategies should live in.

@pollend
Copy link
Member Author

pollend commented Apr 2, 2021

@keturn the annotation is unnecessary and complicates things so I would opt to remove it. in terms of where this fits, probably in gestalt as some kind of utility copy library of sorts.

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.

3 participants