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

Make the skills more flexible #308

Open
wants to merge 3 commits into
base: 1.20
Choose a base branch
from
Open

Conversation

noah1510
Copy link

@noah1510 noah1510 commented Sep 7, 2023

This PR is related to #302.
I went over the code and removed as many hardcoded references to the skills as I found.
This means the following things are reworked:

  • GUIs
  • Skill class
  • Command
  • Networking
  • Data Loading

The only drawback is that the new command implementation also requires you to pass a number to the get subcommand.
If anyone has an suggestion on how make that optional depending on the subcommand that would be appreciated.

Another change is that the order of skills in the inventory is different now.
This is done because before the skills where added column by column and now they are added row by row.
This could be changed by reordering the skills in the Skill class.

One thing that need to be done for this to be actually useful is making the SkillScreen is Scrollable.
Once that is done other mods can add as many skills as they want and everything will be updated as needed.
The buttons get added and work at the moment but they are outside of the window.

Also can someone explain why my IconsStitcher class only works with the main texture.
The correct Identifier gets passed to the draw function and also the correct coordinates but the texture does not appear in game.

@Globox1997
Copy link
Owner

Hey Noah, thanks for the PR but you should have contacted me before you work on such a new system.
My plans are to rework the whole datapack and sync system which will change quite a lot.
About to be on a trip for 5 weeks.

@noah1510
Copy link
Author

If you rewrite those system anyway could you consider some design choices to allow for more skills?
As long as you don't hard code the number of skills and the skill names then everything should be straightforward.
Most of this PR is moving the Skill Information into separate classes to make them more flexible and the rest is basically just removing the hard coded skill names and getting rid of some assumptions when it comes to the number of skills.
If you look into the changes I made they are actually quite small and took only a few hours (including the time needed to figure out how this mod works and how to create commands).

When working on this I also noticed that all things that need to be done per skill are done in separate places (eg. Network sync, data loading, UI building).
Personally I think it would be better to have a skill class that has functions for each of those things and then just iterate over all skill where needed(call sync for all skills, get UI buttons for all skills, ...).
However this would require basically a full rewrite of the mod so I didn't want to do that without knowing if that is even wanted.

@Sewdohe
Copy link

Sewdohe commented Nov 7, 2023

I do agree that skills should be more flexible and would be awesome to be able to add them via a datapack - for example my server running levelZ contains lots of tech mods and I'd love to be able to lock them behind a "tech" skill. Same goes for some magic mods.

Awesome mod though and thanks for all your work! I cannot stress well enough just how much my users have been enjoying this mod on my server :)

@Globox1997
Copy link
Owner

@noah1510 add me on discord Globox_Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants