APDevTeam / Movecraft

The original movement plugin for Paper. Reloaded. Again.
GNU General Public License v3.0
121 stars 76 forks source link

Dynamic Craft Variables #612

Closed goodroach closed 1 month ago

goodroach commented 11 months ago

Is your feature request related to a problem? Please describe. Currently, there is no way to dynamically adjust craft variables such as speed, block percentages, etc. This means any dynamically changing variable such as speed implemented requires a drastic change to Movecraft's base code. I believe this is one of Movecraft's greatest constraints when developing add-ons for Movecraft, as I am not able to adjust variables as I please while the craft is being piloted.

As of current, there are not many dynamically changing variables in a craft. As time goes on, this will surely increase to the point where movecraft will bloat with features just to interact with its current speed variable or other craft-related variables.

Describe the solution you'd like I believe the solution for this is to completely refactor how movecraft interacts with the craft objects by having all the data including all the variables in the BaseCraft and CraftType objects to be included in a single hashmap indexed by NameSpacedKey. This will be very similar to how CraftType records all the individual variables for each craft type. However, in this case, it will be for individual crafts.

Additional notes I am aware of the difficulty of this task, but I believe this will be a great long-term solution for movecraft. I got this idea from C_Corp's PR where he suggested all the craft data to be able to be mapped out in a hashmap. This feature is a monumental task and I believe this will require us to have separate branches in this repository. Another thing to point out is I'm not sure if this system will slow down movecraft as this might increase memory consumption by the plugin.

TylerS1066 commented 11 months ago

I have also identified the addition of a speed API as a big "wish", but you bring up a good point with percentages. Ideally, I'd like to have a cache of the block make-up of crafts, which would allow add-ons to read it and handle sinking (or other behavior) in their own way if desired.

You actually touched on a desire of mine as well, similar to how the CraftType rewrite was utilized to refactor all the internal craft flags to use the same system added for external plugins, we should also use the DataTags system for all internal variables. You mention a performance impact, but the reason for using a NamespacedKey based map is that the performance impact of a look up is normalized to constant time. While there will be some minimal overhead, I'm pretty sure the benefits will outweigh it.

A long term goal of mine is to refactor Movecraft itself similar to how the Movecraft-Combat was rewritten. If we develop enough of these APIs for internal usage, we can move each functionality into it's own contained class file. This will not only drastically help bringing in new devs, it also will heavily reduce the maintenance of current functionality.

goodroach commented 11 months ago

If I were to make this, I will make it so all the craft's type data will be transferred over to the craft file using NamespacedKey in addition to anything BaseCraft or any other of its implementations may have stored in it. Are there any specific parameters that you want to leave untouched? Or would you like for me to develop a prototype already and see if you like my changes?

TylerS1066 commented 1 month ago

With #669 merged, crafts now can have dynamic data. Refactoring speed is another issue logged in https://github.com/APDevTeam/Movecraft/issues/504 and https://github.com/APDevTeam/Movecraft/issues/504