Glad people are interested in this. ^.^ I haven't done any development because collage. But I just had my last day of classes today and will be doing finales next week. After that I'll try to get back onto developing this.
Minetestforfun wrote:Up !
Any news about this framework ?
I'm still alive if that's what you mean. Sadly no progress on my part as I said above.
@TeTpaAka I'll need more time to think about your post fully, but I'll try to address it.
TeTpaAka wrote:
The current implementation has the drawback, that it requires you to improve every time you try your skill since adding 0 experience would mean failure. My suggestion would allow an easy way to check for requirements without modifying the skill. So you could check, if a fighter is experienced enough to enter an arena, for example. The entering of the arena alone wouldn't alter his skill, would it?
There are already functions (getLevel, setLevel, addLevel, and the experience variants) as I mention below. (unless I am misunderstanding you.)
TeTpaAka wrote:
I find the usage of trySkill too complicated. The typical usage of a skillset would be:
- test, whether a skill has all requirements
- do the action
- improve the skill (optional)
With your current trySkill function, this is at least for a single skill possible. But as soon as you've got multiple skills, it gets problematic. For two skills, you can, like in the README, chain the calls, but it would be much easier if trySkill would do, what it is named: Simply try, if the skill works.
The trySkill function may not be the best name for it. My intention was for modders to use the getLevel and getExperiance functions for tests and only run trySkill when they are ready to do 2 and 3. However, now that you bring it up it is not really even needed since modders could just use the get and add functions to accomplish the same thing but with a bit more control. (P.S. The more I think about it the more I believe trySkill should just be dropped to make things simpler. I'll review it.) For example the modder's code could flow like so:
- run getLevel(skill) for all relevant skills and make some tests (RNG, threshold, ect)
- use the data from 1 in their own code to do the action
- run addExperiance(skill) to add the exp amount deserved by doing the action
Note: Apparently I left the add functions out of the README. o.O I'll fix that later. The add functions are just aliases anyway for a setLevel(skill, getLevel(skill)+int) call so it could still be done with the information in the readme.
I would like some feedback on this. Which is preferable. This:
Code: Select all
if (SF.getLevel(entity, "skill_a") == required_level and SF.getLevel(entity, "skill_b") == required_level) then
--do work
SF.addExperience(entity, "skill_a", exp)
SF.addExperience(entity, "skill_b", exp) -- maybe only one skill improves?
end
Or is this better:
Code: Select all
if (SF.trySkill(entity, "skill_a", required_level, required_experience) and SF.trySkill(entity, "skill_b", required_level, required_experience)) then
-- do your work
SF.addExperience(entity, "skill_a", some_experience)
-- SF.addExperience(entity, "skill_b", some_experience) -- maybe only one skill improves?
end
TeTpaAka wrote:
Furthermore, I request the possibility to define different base skillsets. Currently, every call to attachSkillset sets the same skillset for every entity. But maybe it would be easier if you could simply define different skillsets for different classes, so that for example, a dwarf needs less experience to level up in digging. This could be done by adding different amount of experience depending on the class, but it would be less work for the modder if he/she doesn't have to think about the class and simply add always the same amount of experience.
I thought I had this on the TODO list. If it isn't I can certainly add it. I had considered it before, but was waffling on it since I wasn't sure if I should be trying to handle character classes in this, currently, simplistic mod. If I can do it cleanly then I will implement it.
TeTpaAka wrote:
Also it is a bit confusing that the function names are all camel case. In the engine, all functions are lowercase with underscores and the table is all lowercase. I would stick with this for consistency, but it's no big deal if it wasn't like this.
I can change this. I just prefer camel case for functions and hadn't noticed that it conflicted with the core API's style.
TeTpaAka wrote:
But all in all, this is still good work. I could code my suggestions myself, but I don't want to do work which is never accepted. And making a fork is no option since the two skillsframeworks would be incompatible to each other and this is what a framework shouldn't be.
Thank you. :) I will tentatively say that I won't turn away pull requests as long as they are cleanly implemented and well documented. I haven't needed to manage pull request before so bare with me. :P
I hope that answers stuff fairly well and I really appreciate the feedback. If it didn't awnser your q's or you have more thoughts do not hesitate to make another long post. ;)
Edit: Meant to say I'm glad you added the formspec. I was planning on depending on smartFS but also wanted to have a fallback formspec. I may tweak it depending haven't had a chance to look at it.