Use NBT instead of lore
Stumblinbear opened this issue ยท 3 comments
Since RecipeManager uses NBT and lore in a lot of places, can you provide some more detail on what exactly your issue is or what you are trying to accomplish?
Oh sorry about that, I could've sworn I added more than a title. Regardless, I was doing a cursory look through the config and came across the line:
Internally results are tagged with a lore line to identify the recipe
This seems... Like a major hack. Considering (looking through the source, I'll admit I haven't 100% tested everything, as I'm working on other things atm) it's visible to players, it seems like an odd decision to not use an NBT tag so it's invisible to players, and still really easy to access (though admittedly not in as few lines as searching through lore). Also with the fact that you already have tools to access and modify NBT data, not doing it this situation just doesn't make much sense to me.
If it's about speed, perhaps a config value for those that care more about aesthetics and potential compatibility, unless you think that would add too much complexity?
The lore line shouldn't be visible to players (I think it does show up in the vanilla recipe book, but I have little control over that, although I have ideas to potentially fix this) and there's a config option (fix-mod-results) to fully remove it for stacking compatibility if you're running into issues.
As far as I'm aware, you can't add random NBT tags to items and have them be saved to the item and the way NBT data is currently handled (due to limitations with the API and the high cost of full support integrating with NMS) any updates or modifications removes all other effects, making it a very destructive update and making other NBT data nullified.
You're right in that it is mostly about speed, but it's also been around for longer than I have been maintaining the plugin (5+ years) and has been working just fine so far. I'm all for suggestions on how to modify it if they don't affect the performance, but I also don't see much value in messing with it.
A note about the performance point and to clarify what this lore line is doing in case it's not clear: RecipeManager uses built in recipes that get populated in crafting tables (and some other recipe types) and the lore line gets populated on the result, which provides a super fast lookup to find our custom recipe and modify everything from there. This in theory allows thousands of recipes to be created and have almost no impact on the server performance, although I have not tested more than a couple hundred recently.
Another question at this point might be to ask if the lore line ever needs to actually exist past the result (assuming that it does), which would be the equivalent of setting "fix-mod-results" to true always. There's been several major refactors done in the last couple years and I suspect the need for the lore line to continue to exist outside of the crafting grid are no longer relevant. This would require some more digging into the source to find where it's being used and if it's outside of recipe lookups when crafting, which I would hope it is not.