[CF-Devel] button.c magic mouth suggestions
Mark Wedel
mwedel at sonic.net
Thu Sep 12 23:23:59 CDT 2002
Kurt Fitzner wrote:
>>
It should be mentioned what I'm mostly against is using fields of the
>>
object structure to enforce/use some feature which is completely non
>>
obvious. I don't for example have a problem with 'sp' being used to denote
>>
the spell to cast for many objects - sp can at least look like spell.
>
>
>
I'd suggest that this is even more dangerous. We use variables as mnemonics.
>
Having code that uses a variable for two different (but similarly
>
spelled) concepts where the mnemonics are the same is harder to debug than
>
when it's a clear exception.
I think this is probably a double edged sword - using mnemonic variable names
might make it more likely that something gets overloaded for the same use.
However, at the same time, if something is using such a variable, it may be more
likely that the programmer will double check to make sure that it isn't being
used for something else by that object.
I've certainly seen the case where variables of names that are completely
unrelated to the purpose get used for multiple things, causing bugs. Most
likely because the programmer thought 'nothing else is using this odd named
variable, so it should be safe'.
>
Adding more flag bits will only ensure that we have to look at define.h more.
>
And, if you ask me, making define.h the canonical location for all
>
type-specific information is a very good thing. It took me two days of
>
tracing around to figure out how the dragon stuff worked, because nowhere does
>
it say that 'exp' in a dragon ability force means that's what my dragon's
>
metabolism is attuned to.
Lack of documentation is certainly an issue. Arguably, the flag stuff is
poorly done (and I'm the one that is responsible for the current implementation)
- using things like op->removed and op->freed and op->monster, ... and having
those defined as a bitfield would have made things easier to debug.
OTOH, that means the object structure has another 100 elements in it (even if
space is the same), which means when in the debugger you do a print *op, the
amount of information is a lot.
Ideally, having something like uint32 flags[100]:1 or the like would be ideal,
as the FLAG values could just index directly into that, meaning that at least I
don't need to do bit arithmetic when trying to look at the flag values. But
that isn't valid. I'm not sure what the ideal way to deal with flags is.
I don't believe there is any requirement that the compiler merges all those
bitfields to save space - in fact, as I think about it, it is possible that some
compiler options (maximize for speed optimization) but in fact leave each one
aligned on a byte or something simply because access would be faster. But that
then adds 100 bytes to the size of the object structure.
>>
For the other ones, it was done like:
>>
>>
union {
>>
Player *player;
>>
Stationary_Caster *caster;
>>
Monster *monster;
>>
} cst;
>>
>>
And extend as needed. At least in that way, you could look at the type,
>>
see what type of object it was, and look at the appropriate field in the
>>
union.
>
>
>
This works. If we go this route, I'd suggest a union of structs, with one
>
struct per type that needs storage. This will make it a little more readable
>
and make it more clear that this is type-specific behavior.
Yeah. Its sort of odd however - from some perspective, it doesn't make a lot
of sense to add a structure that may just hold a couple values - I guess it
would just seem odd to so. But I guess the use of unions doesn't cost any
memory space, as it is taken care of by the compiler.
Arguably, poor style, but I'd prefer to use an anonymous union like C++
allows, since this union will contain substructure with obvious naming
conventions, the the tag of the union itself is pretty irrelevant (I think I
chose 'cst' above as something like c? sub type (don't remember what the c stood
for).
>
Well, not necesarily. In the case of pointers, it would make freeing up the
>
memory harder, because we'd need all sorts of type-specific code in there. My
>
way, we'd only have to loop through the pointer list and free() up anything
>
that's not null.
True, but that probably isn't too much an issue - new pointers don't get added
very often - most what gets overloaded are the things like ints, which you don't
need to free.
>
I wouldn't worry about having loader.l be type aware. No context sensitivity
>
on a type-by-type basis. For example, an exit in my scheme might have look
>
like this in a map file:
>
>
arch somesortofexit
>
x 3
>
y 6
>
destination_x 12
>
destination_y 4
>
destination_map /some/map
>
end
That works of course, but the fact that all maps currently would have that as:
arch somesortofexit
x 3
y 6
sp 12
hp 4
slaying /some/map
end
So the problem is how do you know that sp could get loaded into destination_x,
hp into destination_y, and slaying into destination_map without knowing that
'somesortofexit' is really an exit?
Obviously, updating all the maps to use the right syntax would fix that
problem. But that may not be very realistic (how do you deal with maps that may
be available for download as 'add ons' but not part of the standard distribution?)
Now you could arrange things so that sp and destination_x (and others) use the
same memory locations. But that seems like a generally bad idea when at some
point they don't match up and people then wonder why something is completely broken.
>
# The following are type-specific values.
>
# These values all decode to the first type-specific int:
>
^destination_x{S} SET_TYPE_SPECIFIC_INT(op, 0, IVAL);
>
^activate_on_push{S} SET_TYPE_SPECIFIC_INT(op, 0, IVAL);
I would think that the hardcoded value of '0' above is for example purposes,
and not the real use? I would think that should always be a #define of some
sort, like
#define EXIT_DEST_X 0
#define BUTTON_ACT_ON_PUSH 0
and so on.
Not sure I see the point of using macros vs accessing the lower memory
directly, like
op->type_ints[EXIT_DEST_X] = IVAL
I'm just thinking the later is a bit more terse but conveys the same information.
>
>
This is actually a bad example, because activate_on_push should be a flag, not
>
an int, but you get the idea. If I had a sword like this:
>
>
arch sword
>
destination_x 3
>
end
>
>
Then loader.l would happily set op->type_ints[0] = 3 in this case, which may
>
or may not cause odd behavior.
Which is probably one good reason why you can't have anything rely on some
specific location (like SP and DEST_X) be in the same location, as someone could
say 'well, it makes sense for dest_x to get set in weapons, so let me put that
at the end of the table).
>
This is a weakness in the scheme. However, I think the net gain in overal
>
readability would be so great, that these types of problems would be rare.
>
And, when they show up, it would be not difficult to detect in the map files.
I do see this case:
Programmer adds some new field to some object, and uses the above scheme. Say
he adds it to weapons.
Someone later then says 'hey - that same value should be used for monsters also.'.
So you now have the case of new_var getting used for two things. The question
of course is whether the developer will do the right thing (which is probably to
move that element name into the top level object now), or do something like set
it so that both monsters and weapons use that same position in the array, but
try to make it so that nothing else uses that location.
I think that this is a cause of a lot of the current problem - do it quick and
fast. Which means not adding new variables. Which means not documenting what
the values really mean.
I note that a lot of people have been very good about documenting what they
do. But the point here is that it is the style of contributors that needs to be
properly enforced.
>
# The following are type-specific values.
>
# These values all decode to the first type-specific int:
>
^destination_x{S}
>
SET_WITH_BOUNDS_CHECK(op, 0, IVAL, MIN_X, MAX_X, "destination_x");
>
^activate_on_push{S}
>
SET_WITH_BOUNDS_CHECK(op, 0, IVAL, 0, 1, "activate_on_push");
>
^dragon_metabolism{S}
>
SET_WITH_BOUNDS_CHECK(op, 0, IVAL, 0, 3, "dragon_metabolism");
>
>
In this case, we could end up with a log entry of:
>
>
Type-specific variable 'dragon_metabolism' in object 'blah' is out of bounds
>
in map '/city/buggymap'
Any method that has the variables in the save files be appropriate unique can
handle the above just fine. But how do you handle all the legacy maps, players,
etc? To me, that is the hard part. If you know that this is dragon metabolism,
even though it is loaded as an 'exp' value, you could still do that bounds checking.
Obviously, this isn't a problem for new values/names that get added. But that
bounds checking doesn't matter if the value is stored in an array, in a
structure unique for the object type, or whatever else, as the bound checking
gets done on the value actually loaded. Eg, it would be something like:
^dragon_metabolism{S}
if (IVAL < 0 || IVAL > MAX_VALUE_FOR_DRAGON_META) do error;
else op->however_this_gets_done = IVAL;
And that first line can just as easily get replaced with a macro that does the
checking/logging without setting value.
I think we both agree that names should be unique for the purpose they have.
And that is enough for bounds checking. The only real issue is how to store the
values.
>
Honestly, I think you're proabably right at this point. If we added a value
>
in for every overloaded value, we might get away with under 256 added bytes
>
per object. Saying that there are 50 overloaded variables is probably around
>
the mark. For 20000 object, that's 5 extra magabytes. But, it's 5 meg we
>
don't need to use. I think the above ideas would be a net reduction in
>
debugging complexity, and it's easily extensible in the future with little
>
size impact. It also starts getting us to use functions or function-like
>
macros for storage and retrieval of type-specific data, which makes moving to
>
dynamic attribute storage easier. Add to that how easy bounds checking is, and
>
how little work it is to start implementing this, and it becomes a
>
good way to ease out of overloading into something more readable and usable.
I will note that if space of objects is any concern, we should look over the
object and see what to clean up. Looking right away, how plugin events are
handled could get greatly optimized. Right now, the object structure is
something like 750 bytes. There are 90 pointers for plugins - 90 * 4 = 360, so
right now, almost half the space is used for plugin support.
Now I think plugins are a great thing, and are likely to get used more and
more in the future. But realistically, what portion of objects will actually
have plugins attached to them? Probably pretty small. So something like put
all those into a structure, and have the object contain a pointer for that
structure. Only allocate the plugin structure if the object in question is
using a plugin.
So in summary:
I think we both agree that overloading the meaning of an element in the
structure is a really bad idea, and shouldn't be done.
We disagree on how to best store specific per type information into the object.
I don't see any easy way to convert currently overloaded meanings in the objects
other than to either have context based loaded/storing (eg, if exit and value is
sp, it goes in dest_x)
So what does that all mean?
It seems that most likely that any such policy mostly comes into play for any
new fields that need to get added.
More information about the crossfire
mailing list