DestroyedClone / AncientScepter

ThinkInvis' ClassicItems' Ancient Scepter Port
0 stars 6 forks source link

Whole refactor #41

Open Anreol opened 7 months ago

Anreol commented 7 months ago

Tried to add scepter to my mod, not only it was hard to see how it's done but also hard how you actually get there, just awful all around, to the point where someone had to send me a code snip to copy and paste. The file organization is awful, there's basically no namespace usage, methods that shouldn't be public are public, and there's absolutely zero documentation comments. Even had some issues when looking at the code through the github repo because everything was everywhere.

The goal is to replace and remove everything that is redundant, simplifying and cleaning code, making the whole project understandable. Old mods should still be able to access the old legacy method to add skills Also adding new features, such as "ReservesASlotNoImplementation" so this mod doesn't do anything, letting the entire implementation be on the other mod's side, but so this mod doesn't delete the scepter item due to it "having no use"

This would be a nice time to add support for fixing #30

Also, while upgraded scepter skills should be their own entity states, instead of hacking the original skill's state with some hooks, as other mods (ie. RiskyMod) might touch on those, so scepter skills should be standalone, that seems like too much work for me right now. Additionally, moving content such as SkillDefs to the Unity assetbundle so they don't have to be entirely created through code would be nice, but I haven't even opened that yet, and I'm not sure if it works.

Making this draft pull request to gather feedback and so other people might help with this.

yekoc commented 7 months ago

Hook to proper New Skill conversion checklist.(I'll handle em)

Anreol commented 7 months ago

Oh also, I haven't written anything related to this in the code, but I thought that it would be more proper to have entirely new States (ie. inside EntityStates) for each Scepter skill, inheriting from the State that they are upgrading, so they have the ability to have their own Entity State Configuration file, and maybe to override or append some behavior, too.

EDIT: Additionally, doing that would make each upgrade "standalone", meaning that any changes to the basic skills's entity state configurations from other mods wouldn't affect Scepter's, and at the same time it would open up the possibility of people modding the upgrades without affecting the basic skills.

yekoc commented 7 months ago

That was the intention I was going for when I put up the checklist,indeed. :^) Though it could be argued that balance changes from other mods propagating to the upgrade is a Feature™ instead of a bug.

Anreol commented 6 months ago

Been busy with work, might work on it next week since I have some holidays, but at the same time: holidays, so I can't make any promises.