A little feedback

[tekproxy]

I have some free time with it being nearly christmas and all so I've been doing some housekeeping around the distro, trying to finish it up!

One thing I noticed is a lot of bug reports / patches, etc... and that is just great. Thanks a lot for helping. It's always appreciated and sending patches is a good way to get experience scripting.

Who watches the distro or uses a modified form of it and stays current with the SVN?

What are some of the main things you want to see finished?

The main thing on my mind is how skill checks and skill gain work. Right now it's a little weird and is compounded by the fact that I didn't write it and it's a complex system (attributes). I think it should be configurable how someone wants skill gain to work. Should it progression be easy or hard, linear or geometric?

[dkpat]

what i want to see most of all, is the rest of the skills scripted.

[OldnGrey]

I regularly check the distro for changes.
To me the distro represents the state of the art scripting for pol097 and I have studied and copied many of it's ideas. Anybody who starts scripting in POL will want to use it, but obviously with it being incomplete the decision for the new scripter has to be about what script set to use.

I think it's the case that a lot of new scripters still pick up the pol095 scripts because they are still the most complete and sadly there is a huge gulf to bridge to bring them up to the 097 distro later.

I would dearly like the foundation systems checked for consistency. In particular the attributes system seems a little too complex and seems to be written in a way that makes it really hard to include IntrinsicMods. I really don't know why the distro can't simply use GetAttribute() instead of making its own functions to do the same thing.

However, I do believe that eye candy items are important in the distro. Things like the new spell books and the fight book. Things like that really to illustrate the 097 is for Mondain's Legacy and not a re-write of pre-AOS.

(I intend to tackle BrainAI again, but it's so heavily dependent on TimedScripts and the Attributes system that it's hard for me to work on it. I did submit a function to start/stop the buff/debuff packet, but that's as far as I can take it while I still have a shard that's not 100% distro compatible)

[tekproxy]

what i want to see most of all, is the rest of the skills scripted.

Skills will come along much more quickly once the dependent systems are put into place.

I would dearly like the foundation systems checked for consistency.

I think I know what you mean, but could you elaborate?

Like I said, the main thing I want to do is go over the attributes package and tweak things so it is easier to use. Right now it seems like skill gains are too fast if you check a skill against itself. I spent many days staring at it and trying to figure it out but I was slower then maybe I can get it now.

The reason those functions exists instead of just calling GetAttribute() is so there is one unified place for all of that, it's a layer of abstraction. This way, if you ever wanted to change how you record attributes, you could change it in one place. If the core ever changed how the GetAttribute() function worked, you would only have to change it in one place.

[Austin]

For attributes, we have the functions to handle the division and multiplication to allow for 10.1 10.2 10.3 etc.

To adjust the skill gain rates, look for the settings.cfg in the attributes package.

[OldnGrey]

Only a couple of comments on the attributes system because to me it's still a very complex system and I don't fully understand it.

1. SkillCheck (a minor point)
This essential function is used by nearly every skills script, and yet it's not called AP_SkillCheck. It's not like it's a helper function like CalcSuccessPercent, it's a major function replacing the core's CheckSkill which was reliant on skillid.

2. AwardPoints / AwardRawAttributePoints
These functions used to exist on the 095 distro and were very useful. There is no equivalent way in the 097 distro to add add a tiny skill bonus. I can't see SkillCheck replacing that in every script situation. When the distro went away from the GetNeededRawPointsToIncrease function we lost a way of awarding a small skill bonus that was less than 0.1 skillgain. The 095 checkskill used to call AwardPoints and gave us an interface to awarding small amounts of skill that could accumulate. I actually liked the old points system that converted skill values to a points system and made each skill increase harder than the previous one.

3. The attributes package lacks support for intrinsicmods. I realise it was almost removed from the core a little while ago, but it's there now and very useful when wanting to have race skill bonuses. To that end several new functions would have to be written into the attributes package (skills.inc) such as
AP_GetIntrinsicSkillMod (replaces GetAttributeIntrinsicMod) Returns intrinsic mod as a floating value eg 2.5
AP_GetSkill would need to be modified to also add in the intrinsicmod
We also need AP_GetTrueIntrinsicSkill (or a better name) that retrieves base value plus instrinsic mod and returns a float eg 102.4 (being AP_GetIntrinsicSkillMod plus AP_GetTrueSkill) This function would be used instead of AP_GetTrueSkill whenever calculating a player's race skill value which is most of the time.
To support intrinsicmods you will also need to add another support function and actually calculates the intrinsicmod. I've called it intrinsicmods.src and put it in the attributes\hooks directory. It is called from the attributes.cfg file - eg for the alchemy entry it is:
GetIntrinsicModFunction :attributes:hooks/intrinsicmods:IntrinModAlchemy
The intrinsicmods.src script contains exported functions for all the skill attributes and is setting driven to determine the mod based on a player's gender or race (gender values supported by OSI are currently 0, 1, 2, 3 so at least you can code in elves skill mods if you want)

With attributes, I don't think I am remaining locked into old ways of thinking. About all I can say is that it will be a very hard task indeed to convert any existing shard to use the new attributes system. IntrinsicMods are not essential in that respect, but the functions available in the attributes package do not seem to be complete especially when it comes to awarding small skill gain.

[OldnGrey]

Also AP_GetSkill (skills.inc) is supposed to return an integer, but returns a dbl instead.
To reflect the documentation it would have to be:

Code: Select all

function AP_GetSkill(who, skill_name)
  return CInt(AP_GetTrueSkill(who, skill_name) + AP_GetSkillMod(who, skill_name))
endfunction

instead of

Code: Select all

function AP_GetSkill(who, skill_name)
  return AP_GetTrueSkill(who, skill_name) + AP_GetSkillMod(who, skill_name);
endfunction

[OldnGrey]

I've spent some more time on the attributes package and would also make the following suggestions:

skills.inc
AP_GetSkill can simply be:

Code: Select all

function AP_GetSkill(who, skill_name)
	return GetAttribute(who, skill_name);
endfunction

stats.inc
AP_GetStat can simply be:

Code: Select all

function AP_GetStat(who, stat_name)
	return GetAttribute(who, skill_name);
endfunction

The code already in the package calls too many functions and converts to CDbl to handle division by 10 just to calculate an integer. There is already a core function which is presumably much faster to handle that already.

[OldnGrey]

If you really want to fix up the attributes package, then take a look at the SkillCheck function inside attributes\include

The actual pass or fail of a skill is calculated here but not communicated at all to the CheckSkillAdvance function.

To see what I mean look at what happens to the check_roll variable.

[OldnGrey]

And to top off the Attributes system problems, here is another nice little bug:

attributes\include\titles.inc

look in function AP_GetLevelTitle

Code: Select all

var levels_list := GetConfigStringDictionary(title_elem, "Level");
foreach entry in ( levels_list )
	if ( skill_level < CDbl(entry) )
		return _entry_iter;
	endif
	SleepMS(2);
endforeach

The problem with this is that the levels_list dictionary is actually sorted when read in, and it isn't in the same order as they were in from titles.cfg.

That means a skill_level of 0 actually returns as "Amateur" because that is the first entry in the levels_list. The order of the entries in titles.cfg is crucial for the maths of this routine.

I don't have an elegant solution I'm afraid apart from forcing the file to be read in order by structuring it like this:

titles.cfg

Code: Select all

Level	a  10	unskilled
Level	b  20	Beginner
Level	c  30	Amateur

And then using something like splitwords to get the real parameters: eg

Code: Select all

foreach entry in ( levels_list )
	var the_string := splitwords(entry);
	if ( CInt(skill_level) < CInt(the_string[1]) )
		return CStr(the_string[2]);
	endif
	SleepMS(2);
endforeach

Sorry that fix is so ugly.

Hopefully a better scripter will find a nicer solution.

[OldnGrey]

I see Austin has done a nice fix for the titles.
Now here's another one.

The core double-precision bug is still there and it makes a difference in the 4 Attributes functions that set stats/skills. As the functions currently exist, try setting a skill or stat to 0.8 and you will get 0.7.

AP_SetTrueSkill
AP_SetSkillMod
AP_SetTrueStat
AP_SetStatMod

Although the advancecheck.inc file deals with the bug internally (CheckStatsForSkill and CheckSkillAdvance), these aren't the only scripts that set the value of an attribute using one of the above scripts.

I'd suggest doing the mods in the 4 functions above, then any script can set a skill or stat value safely.

For example change AP_SetStatMod to read:

Code: Select all

function AP_SetTrueStat(mobile, attributeid, points)
	if ( points >= 0 )
		SetAttributeBaseValue(mobile, attributeid, CInt(CDbl(points) * 10.0 + 0.1));
	else
		SetAttributeBaseValue(mobile, attributeid, CInt(CDbl(points) * 10.0 - 0.1));
	endif
	RecalcVitals(mobile);
endfunction

[OldnGrey]

I got to thinking a bit more about the double precision thing.
When you CInt(CDbl(n)) you seem to often end up with a lower number than you thought. With the emphasis in the Distro on doubleprecision numbers in the attributes, take a look at vitals.inc and timercontrol.src too.