-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Spec] Additions, Removals, and Deprecations #76
Comments
The `ver` attribute was never actually formalized and just meant as a shorcut for using both ver1/ver2 on the same version. `userver` was almost never used and `userver2` was becoming increasingly uncommon as most comparisons were switched to inequalities. To make things easier, these should just be part of the expression.
…ols#76 The `cond` attribute was being overloaded with multiple functions. Conditions should be actual expressions, using local or inherited data. Type checking inside of `cond` would also mean that you could do both in one expression, such as `cond="NiParticleSystem && Num Vertices > 0"` which doesn't make any sense. So, `onlyT` and `excludeT` were made to serve that function. Two attributes also means not having to use the unary not (!) in order negate the meaning. Existing logic can simply alias the attributes to cond if need be.
For niftools#76 `bool` type is something that has to be dealt with as it changes size. Also it's marked as countable which should probably be analyzed... It shouldn't be used in array counts.
Was completely unsupported and undocumented and unused by any software. Replaced with arithmetic even though the decoding is likely wrong anyway. For niftools#76
For niftools#70 and niftools#76 Adopting the token syntax of the other tokens, TEMPLATE and ARG can now be treated the same as other tokens and differentiated from regular identifiers in a generic manner.
For niftools#76 Also reorganized the basic types to be in a logical order.
For niftools#76 The signed integeral types were marked as countable even though they should not be used for array sizes. A negative value cannot be passed into an array size. They are still integral though, so countable was split into `countable` and `integral`. In addition, added `boolean` to designate types that can toggle the presence of another member. Also reordered items for `convertible` so that the referenced types come before the type referencing it, i.e. larger types first.
For niftools#74 Implemented thanks to changes done in niftools#76 for differentiating integral and "countable" i.e. can be used for sizes. Marking types as boolean was also in niftools#76, yet there were no errors for this lint; all the `cond` references that were not complex expressions were already boolean types.
For niftools#3, niftools#76 Replaces all Flags types with real enums (where there were only two options) or a new bitfield type. Bitfield mimics the bitfield syntax of C structs (Compounds), but also acts like Enum/Bitflags with the `storage` attribute in that the entire structure can simply be aliased to a basic type like `ushort`. The `type` on each Bitfield member does **NOT denote size**. Each member only takes up the bits that are defined by `width` and `pos`. The `type` information is for casting for getters and setters. That means for C structs, code generation would not use the type on each member, but the storage type instead.
I won't repeat the entire thing here, but I detailed the usage of |
For niftools#76 For reduced ambiguity. Adapting for this in Python is as easy as ```py # XML Booleans XML_TRUE = ["true", "1"] XML_FALSE = ["false", "0"] # Example self.is_abstract = get('abstract') in XML_TRUE ``` Note "True" and "False" are not valid booleans in XML. Only lowercase.
For niftools#76. `calc` does not need to be supported and is only used for pre-serialization preparation of the data. This can also be done by hand. Some revisions also need to be made for niftools#70 and niftools#73 to account for another attribute with its own expression grammar and tokens. However, the tokens introduced for `calc` will not be added to the tokens in the XML and will need to be explicitly supported if supporting `calc`. They are: 1D array size (uses regex): `#LEN[(.*?)]#` 2D array size (uses regex): `#LEN2[(.*?)]#` Ternary `?`: `#THEN#` Ternary `:`: `#ELSE#` Also removes any unnecessary `calculated` without replacement. Additionally, `calc` is used to limit array size for the time being, though it's possible this should be done with its own attribute, such as `maxlen`.
I added a section titled Defaults of inherited values for My thoughts: The attribute method is easier to implement and ignore, but harder to actually support. The element method is strictly XML with no special parsing required and as child elements of I'm guessing I will implement it as the element method. As for the defaults being centralized instead of associated with the subclass, I can always document the defaults in the docstring. |
Also one problem with the centralized declaration I just thought of is that for <add name="Order" type="uint">
Modifier ID in the particle modifier chain
<default type="NiPSysEmitter" value="1000" />
<default type="NiPSysDragModifier" value="4000" />
<!-- -->
</add> The However in this case, the data here is metadata. It is not actually required for the class it is declared in, but stored and used later on for the declaration of its subclasses. Also not handled in the issue text is the declaration of the base default. I suppose either of these works, and maybe both should be valid: <add name="Order" type="uint" default="3000">
Modifier ID in the particle modifier chain
<default type="NiPSysEmitter" value="1000" />
<default type="NiPSysDragModifier" value="4000" />
<!-- -->
</add> <add name="Order" type="uint">
Modifier ID in the particle modifier chain
<default type="NiPSysModifier" value="3000" />
<default type="NiPSysEmitter" value="1000" />
<default type="NiPSysDragModifier" value="4000" />
<!-- -->
</add> Also, validation considerations, re: #74
|
I added a section titled
|
For niftools#76 Implement initial subclass defaults. Had to add additional attribute to deal with versioning of defaults, like block versioning. Added more defaults to normal fields. Added default to Vertex Color array, also enforcing that members with `arr1` can have a `default`.
For niftools#70 and niftools#76 Added tokens for range attribute strings. Did initial pass of ranges, many from Bethesda's official export scripts for FO4, which limit the values in the UI. Also have begun using data analysis to find implied min/max ranges (as well as sane defaults).
For niftools#70 and niftools#76 Added tokens for range attribute strings. Did initial pass of ranges, many from Bethesda's official export scripts for FO4, which limit the values in the UI. Also have begun using data analysis to find implied min/max ranges (as well as sane defaults).
This is a catch-all for attribute changes and their discussion that are not covered by other tickets.
Replace/Remove/Rename
General
true
/false
should be used over 1/0. They are valid in XML as well, and more clear as to their intent for vague attribute names.TEMPLATE
andARG
with#T#
and#ARG#
, to match with [Spec] Token definitions #70'Flags' type removal
As discussed in #3, a replacement for
Flags
is desperately needed. Instead of rewriting the issue of that ticket I will put everything here with the rest of the removals and changes.While the comments of issue #3 were very informative for the concept and possible solutions, we were all trying to change
<bitflags>
to suit our needs. A bitflag is an enumeration, and in languages like C++ is able to be represented with an enum, except instead of 0,1,2,3, the values go 0x1,0x2,0x4,0x8, just like we are doing with ourenum
andbitflags
.What we were really discussing was bitfields, which are separate from enumerations. They are simply a struct, but with a bit spanning syntax. In my 010 editor templates (C-like), a bitfield looks like this:
A bitflag can also be represented in this way, but only with
: 1
at the end of each statement. (I will note that in 010 editor I also used bitfields for bitflags, but simply due to the way the UI treats enums.)The current proposed syntax is:
For C/C++ and other languages with actual bitfields, only the name,
storage="ushort"
, andwidth
are needed. Thetype
shown here for each member is NOT used for (de)serialization, it is more or less the return type for getters and the input type for setters for methods to set the members.For anything else that cannot use just the width for each member, the position and mask have been included. These allow you to retrieve the value for the member from the
ushort
directly.Getter
Setter
Where
flags
is the entire ushort, andvalue
is the value of the particular field.<add>
Remove:
userver
anduserver2
These two attributes had minimal remaining use. I've already switched their use to the format
vercond="User Version == ..."
. Also,userver2
was never technically formalized afaict. It was not supported in every project such as niflib, where nifxml.py/gen_niflib.py seem to have no support for it.Remove:
arr3
There is a single legacy undecoded object using arr3 for some reason, and it is otherwise undocumented and unsupported by all programs. It only seems to exist due to lack of arithmetic in the arr expr, which exists now. So it is being removed in lieu of that.
Rename:
ver1
andver2
For block versioning, I have adopted
since
anduntil
and this is succinct and human readable. The nice would be the same for<add>
. However there is also alternatives likeminver
andmaxver
. But ver "1" and ver "2" inherently mean nothing.Replace/Remove:
calculated
I've talked about this attribute a lot and still am completely in the dark about its intent. If it does what I think it's intended for, it should be on way more fields than the 3 that it is present on.
For most things, there is a simple two-way boolean relationship for example:
I don't really understand the need for
calculated
here. At write time, if the Triangles array has a length, you know that you must set Has Triangles to true. If the Triangles array has no length, it doesn't matter whether or not Has Triangles is set to true, unless for some reason the program has erroneously set a Num Triangles but doesn't have any Triangles in the array.Where it's actually needed is in examples like this:
You can calculate the Data Size values from the content after them, but in this case, the data before Data Size has to be in agreement as well.
Except... either size can also be 0 even if Vertex Desc, Num Triangles, and Num Vertices are all non-zero. So, really the
calculate
field would only be a guideline on how to get the correct value, not a rule that is always followed.Replace
abstract
NiDataStream has two members which do not contribute to serialization. This is done because
NiDataStream
RTTI string has two arguments packed at the end so they are effectively the first two members of the NiObject, just packed with the name instead of the block.The name
abstract
is confusing for many reasons, but it's simply factually incorrect in OOP, which the XML is emulating.As for alternatives, hkxcmd uses
flags='SERIALIZE_IGNORED'
for example, but it also has a lot of flags for its class members. I also think using an attribute would be kind of hard to see and a separate tag name would also enforce treating them differently so that there is no mistaking that the data need not be serialized.So, the obvious choices would be to have tags named
<property>
/<prop>
or<metadata>
/<meta>
.For existing parsers or codegen, this has the benefit of not confounding members that serialize vs members that don't, as they simply will not see the new tag. Right now, anything that doesn't pay attention to
abstract
on members will erroneously try to read/write the values from disk.Replace
cond="T"
andcond="!T"
Several instances of
cond
involve checking against the type of NiObject the member is being included in such asor for NiGeometry
Mostly in order to fix inheritance issues caused by historical changes to class hierarchies, or in Bethesda's case, entire class replacement (NiGeometry->BSGeometry). Since we have no mechanism for multiple niobject definition with inheritance changes, we settle for customizing the content of the parent niobject based on the child type.
This mechanism is not an expression though, and as such does not belong inside of
cond
. The names of NiObject types should not appear in cond as they are not local information.The replacement would be:
or for NiGeometry
Two separate attributes will set it apart from normal
cond
, which will no longer have to be parsed to see whether or not any part of the expression is an NiObject and whether there is a unary not (!).For existing applications, if rewriting logic is undesirable,
excludeT
andonlyT
can simply be aliases forcond="!T"
andcond="T"
respectively.<basic>
Rename
count
The function of this initially eluded us until I saw the connection between the types that have it, and then eventually noticed that the language in the docs generator supported this. Basically, it means "integral", but should mean "countable". It doesn't technically mean countable because the
int
basic type is set as countable as well, even though signed ints should not be used as counts.The phrase "count" is also too close in meaning to "size" or "length", which may also be introduced as an attribute for basic/compound types, and the use of 0/` instead of false/true also further confuses the intent.
Additions
<basic> and <compound>
int64
anduint64
typesA few types in the XML are really bitfields with a 64-bit storage type, and at least one field is potentially a 64-bit hash. So the addition of 64-bit types is necessary.
size
The basic and basic-ish compound types have no information on their size for read/write. Objects that are always a fixed size should declare their size in the XML.
casts_to
/convertible
The other types that it is acceptable to be cast to (where information is not lost), e.g. short->int or ushort->uint. This is necessary for deducing if version-exclusive duplicate names of differing types are true duplicates or not. Or if the members are in an incorrect order, e.g. for ushort->uint, the uint has to be listed first.
It also is useful for defining what compound types must have a conversion function available, such as
HalfVector3
->Vector3
.For example:
These do not need to be treated as different values for codegen or anything else because a ushort can be held inside of a uint. However if the members were in reverse order, the uint could not be fit into the ushort without potential information loss.
For Python, this doesn't really matter much, as there is no real size limit to integers, but it does affect the logic during (de)serialize. It will also aid in linting.
Example of usage:
The opposite direction of this could also be used instead
This would basically be a list of all the types that can fit inside it.
If the basic types also had their
size
attribute, this might not even be necessary.Split
countable
intointegral
,boolean
, andcountable
Currently the signed integer values are marked as countable, but really shouldn't be. You can't have a negative value for an array length. The type is still integral. Furthermore, it's not boolean. The only types that should be considered boolean are
byte
andbool
. Boolean types should be the only types that can toggle the presence of a sibling member. Addingintegral
andboolean
will help sort out typing issues during linting for array and cond references.In Summary
False
implies countable and boolean are false as well.True
should be the only way a member can toggle the presence of another member.<niobject>
Defaults of inherited values
For each niobject, there can be only one default for a member, in the niobject that it is declared on. However, several Gamebryo classes modify inherited defaults for each class type. Some examples:
Order
Flags
defaults, such as NiNode vs BSFadeNode vs BSBlastNode/BSDamageStage.So to provide proper defaults for subclasses, a syntax is necessary for overriding default values of inherited members.
Using a Pythonic dictionary syntax:
Downside: Other than Python using literal eval, this syntax would have to be specially parsed. A native XML syntax could be used, but it would be more verbose and introduce more tag names.
Example:
Downside: NifSkope currently actually errors if anything other than
add
exists inside ofniobject
etc. so simply adding a new child tag would not be ignored. This is a minor downside, and can quickly be fixed, but just pointing out that new tags are not necessarily ignored by default.Organizationally, a heterogeneous mix of child tags is a bit odd for nif.xml but not XML in general. The default metadata could also be supplied alongside each subclass, or in the parent itself.
In the parent:
Upside: This centralizes the info and it's not strewn across the subclasses.
Downside: This info is not apparent when looking at a subclass. 🤕
Note: For this particular member, there may be more than a dozen actual defaults listed.. not just the two shown in the examples.
Lastly, I mentioned that it's technically metadata i.e. not directly vital to (de)serialization which is true, but it is still vital to data correctness. That is why the last option of putting the info alongside the niobjects instead of inside is the least appealing to me. It would essentially be a type-name-value dictionary in XML so I won't bother with an example.
<add>
range
(numeric limits)As of writing this, I use
calc
in a few locations to limit the final size of aNum
value. This is fine, but in the general case, some values need to be enforced to a very limited range of their full storage size. In some cases (such asNum
values), the only limitation is that the value must be at least 1. Sometimes it is an upper limit. I feel that having an attribute for both the lower and upper limit is excessive and instead the Python range syntax can be borrowed.Right now, I've begun marking some of these fields as
default="1"
because there should always be at least one, but this isn't actually an enforcement.For the first two,
default="1"
is technically redundant. Since the default for auint
is0
, then any min of non-zero automatically becomes the default. However, existing systems already rely on thedefault
attribute for member initialization, and the number of fields that will havedefault="1" range="1:"
should not be too high. This opinion may change though.To not write both, any non-zero min of
range="X:Y"
would have to implydefault="X"
and any parser would have to derivedefault="X"
on its own.const
(immutable values)Some class members should be set at creation time and be immutable, i.e. not changeable by a user or any other means. For example type info that is tightly coupled with class type.
A
default
value is obviously required whenconst="true"
.<enum> and <bitflags>
Bit Patterns in bitflags
Several bitflags have default attributes set with plain integral values, and this doesn't allow anyone to easily see which bitflags options these are. For example:
Default fields don't support expressions, and so a token with the various options BITORed together would not help. It could instead be a feature of the option:
Or so. Each value would get split on
|
and then shifted and BITORed. This way, using actual enumeration values as defaults could be enforced for bitflags as well.Clarifications
Some things remain unclear about nif.xml and need to be stated explicitly
<add>
default
usage for arraysNowhere is
default
used alongsidearr1
. However there are some arrays where a default should be provided. The default would apply to any members added to the array of that type.Example:
The default color for the Vertex Colors array should be
1.0, 1.0, 1.0, 1.0
.For Discussion
Like other tickets, this isn't completely finished and I may be adding more as time goes on. I will be making commits for this ticket fairly immediately however.
Checklist
ver
with equivalent ver1/ver2 statementuserver
/userver2
arr3
cond="T"
andcond="!T"
withonlyT
andexcludeT
size
to appropriatebasic
andcompound
TEMPLATE
andARG
int64
anduint64
countable
intointegral
/boolean
/countable
.convertible
for types that can be cast to a larger type without information loss.bitfield
tag.calculated
abstract
for<add>
e.g.NiDataStream
"Usage" and "Access".true
/false
for clarity.niobject
inherited value defaultsrange
attributeconst
attributedefault
for array values where applicable.The text was updated successfully, but these errors were encountered: