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 dashloader support. #30

Closed
wants to merge 1 commit into from
Closed

Add dashloader support. #30

wants to merge 1 commit into from

Conversation

alphaqu
Copy link
Contributor

@alphaqu alphaqu commented May 24, 2021

Added the proxy model with the serializer annotations and how DashLoader should handle the models

@alphaqu
Copy link
Contributor Author

alphaqu commented May 24, 2021

Unsafe lib got added to dev env because of some wierd issues with gradle not getting the dependency from DashLoader.

@FoundationGames
Copy link
Owner

This seems to be quite an interesting implementation.
I skimmed through the code and found a few uses of if chains, which should be removed in favor of a registry-like implementation for ModelSelector and related. I can implement this myself post merge, however, and will try to do a more thorough and proper review of the code later.

@alphaqu
Copy link
Contributor Author

alphaqu commented May 24, 2021

Please note that this is on the serialization stage which is not at all important, only the undash methods are important in terms of performance as they are what get used on the loading

@FoundationGames
Copy link
Owner

If chains and switch cases (when not used on things like enums) are unclean and make it tedious to add models in the future, which is why the current registry systems exist.
I would much rather have a registry system for those objects that you have assigned IDs in those methods.

@alphaqu
Copy link
Contributor Author

alphaqu commented May 24, 2021

Oh you were talking about switching the entire system out for a better one, sorry for the misunderstanding.

@alphaqu
Copy link
Contributor Author

alphaqu commented Jun 30, 2021

Whats the status of this being merged?

@FoundationGames
Copy link
Owner

Currently busy with other mods atm.
Keep in mind the next release of EBE will be for 1.17, and I'm not sure if dashloader is updated or not or whether this PR will require changes.
Along with this the PR will also change my general workflow when testing the mod as well, and I am not sure whether this will impede my ability to test the mod in vanilla (without dashloader)
I will consider merging it after the 1.17 release of EBE.

@alphaqu
Copy link
Contributor Author

alphaqu commented Jul 3, 2021

Dashloader was among the fastest to update to 1.17 and this impl is designed to work without dashloader.

@ghost
Copy link

ghost commented Oct 13, 2021

how to download
and when will it be implemented?

@he3als
Copy link

he3als commented Jan 7, 2022

When will this be implemented?

@alphaqu
Copy link
Contributor Author

alphaqu commented Jan 23, 2022

very out of date

@alphaqu alphaqu closed this Jan 23, 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
Development

Successfully merging this pull request may close these issues.

3 participants