Glitch Wars
<i>This post was originally made by <b>skyjake</b> on the dengDevs blog. It was posted under the categories: Engine, Games, Mac OS X.</i>
The most obvious glitches remaining on the Mac are:
<ul>
<li>In all games, line hit tests are evidently hitting non-solid lines. Punches will hit two-sided invisible lines between sectors. This also seems to mess up the Use key because I can't open any doors; most likely the used line is misdetected? Oddly, this wasn't happening until after the invisible walls problem was fixed.</li>
<li>3D models are invisible. When rend-model is disabled, all mobjs seem to be visible like they should.</li>
</ul>
BTW, the menu selection arrow is still mis-aligned in jHeretic and jHexen.
The most obvious glitches remaining on the Mac are:
<ul>
<li>In all games, line hit tests are evidently hitting non-solid lines. Punches will hit two-sided invisible lines between sectors. This also seems to mess up the Use key because I can't open any doors; most likely the used line is misdetected? Oddly, this wasn't happening until after the invisible walls problem was fixed.</li>
<li>3D models are invisible. When rend-model is disabled, all mobjs seem to be visible like they should.</li>
</ul>
BTW, the menu selection arrow is still mis-aligned in jHeretic and jHexen.
Comments
The 3D models problem is odd considering mobjs appear normally when they are disabled... Again I'll look into that.
Menu selection arrow mis-alignment - I'll fix that today.
I think we need to revise the propertyTypes array completely. It is very easy to make mistakes in the types and cause many problems that may not immediately appear on big-endian systems. We need something that's easy to maintain and also fool-proof so that the type of each data member is defined only once. Perhaps a set of macros that are used both in the engine's p_data.h and dmu/arch? That approach may get a bit tedious, though.
We could at least in theory define a set of structs that represent each data type (say, DDVT_SHORT) and then use those to replace all the normal data types in the p_data.h structs. But our dd_short_t would have a header field that would define its DDVT-type. That way it would not be possible to misdetect the type, since it's stored in the value itself. There is some memory overhead in storing the value headers, though... Comments?
I'll have to keep investigating the 3D model problems. There has been problems rendering them in the past on the Mac.
Now that I look at it again I can see whole host of things I've done poorly and some stuff that I've not addressed that is now unnecessary (frametomap coord conversions etc).
This revised code should work 100% correctly though after beta4 is released I'll go back and rewrite the automap from scratch.
<blockquote>The line hit test thing was caused by DMU's GetProperty, where the propertyTypes array incorrectly defined that a line flags' type is DDVT_INT when it really is a DDVT_SHORT.</blockquote>
Glad to hear it's now fixed.
<blockquote>I think we need to revise the propertyTypes array completely. It is very easy to make mistakes in the types and cause many problems that may not immediately appear on big-endian systems. We need something that's easy to maintain and also fool-proof so that the type of each data member is defined only once.</blockquote>
I agree.
The macro approach would indeed get tedious but I think it would be preferable than passing the overhead on to users in the form of value headers.
How about a dictionary style approach where each data member accessable via DMU/DAM registers itself at runtime by DMU/DAM constant an object type constant and a value type constant? Too much? Just as inefficient?
<blockquote>I'll have to keep investigating the 3D model problems. There has been problems rendering them in the past on the Mac.</blockquote>
I've not thought of anything yet. I did add mobj fade targets to the code which control the transparency of mobjs but as you say sprites arn't affected by this bug then I can't see it being that(?)
The 3D model problem was a non-issue. I had simply forgotten to load a PK3 containing the model data files, as my development source directory only contains the definitions at the moment.
I did notice a new problem with jHexen. Try this:
<ol><li>Start a new game.</li>
<li>Warp to map 3 (or any other map).</li>
<li>Warp back to map 1.</li>
<li>P_ToPtr() reports an error about invalid object type, probably because when the map was loaded some objects were not correctly initialized.</li>
</ol>
Can you reproduce this on Windows?
Yes a compile-time sollution would be best I think. I've been looking at the OOPC project for ideas on this.
<blockquote>The 3D model problem was a non-issue. I had simply forgotten to load a PK3 containing the model data files, as my development source directory only contains the definitions at the moment.</blockquote>
Good to hear it wasn't a bug anyway.
<blockquote>I did notice a new problem with jHexen.... Can you reproduce this on Windows?</blockquote>
Yes I get the same on Windows. I'll see if I can find the problem.
Another glitch I've found is due to the changes I've made with map meta data being removed from the source and always loaded from DED and the logic used to determine what to use for the map name in both the automap and the map title displayed at the start of the map (all games).
I changed the behaviour so that if a PWAD contains a custom level name patch graphic (normally used for the intermission) then it would display that instead of the map name string as set via DED. The logic doesn't appear to be quite "right" atm.
I'm thinking the following is the correct order of priority:
<ol>
<li>Name String (MapInfo Def)</li>
<li>Patch in PWAD</li>
<li>User defined Patch replacement string (Values)</li>
<li>Built-in patch replacment string</li>
<li>Patch in IWAD</li>
</ol>
With the top two cases needing to be reversed if the level the patch is for is not from an IWAD. Correct?
Issues/tasks remaining to my knowledge:
<ul>
<li>Object initialization problem? jHexen warp problem (DMU/DAM)</li>
<li>With GLNODE buidling disabled Doomsday currently crashes during startup</li>
<li>Current light range is reset when when changing sectors.</li>
<li>Tweak the lightrange value defaults?</li>
<li>"MISSING" texture binding not working correctly</li>
<li>XG is currently untested since the update to DMU</li>
<li>Menu cursor postion in jHeretic/jHexen</li>
<li>Level name position in the automap</li>
<li>BIAS lighting glitches:
<ol>
<li>lighting is not consistent between onesided seg middles and twosided seg uppers+lowers.</li>
<li>surface colors are not intergrated with BIAS, twosided seg middles are currently hidden to remind us of this</li>
<li>Lightning in jHexen causes massive slowdown due to having to change the light levels of all outdoor sectors (lightgrid update). It might be possible to implement this using a value shift in the lightRangeModMatrix depending on if the sector has a sky flat instead of actually changing the sector's light_level (would that be OK or do we need to do it properly?)</li>
</ol>
</li>
</ul>
Obviously there is still a lot more work to be done for the final 1.9.0 release but these need to be done for beta4.
Any further issues/tasks you can think of skyjake?
Namely, the XG code in 1.8.6 appears to have freely used fixed_t and int interchangeably, for example the sectorinfo_t members origfloor, origceil etc all use int instead of fixed_t and their values are assigned to using implicit casts.
Naturally this means problems since DMU will not cast fixed_t to int and instead expands FRACBITS.
Rather than change the data types of these members at this stage I think it would be better to change each case where the values are referenced/assigned using explicit casts eg
int ffloor = (int) P_GetFixedp(sec, DMU_FLOOR_HEIGHT)
Then after beta4 is released I go back and fix them properly? Or would you rather I just fix the type usage outright, for beta4?
<blockquote>int ffloor = (int) P_GetFixedp(sec, DMU_FLOOR_HEIGHT)</blockquote> Fortunately, int == fixed_t. So, the following should be fine:
<pre>int ffloor = P_GetFixedp(sec, DMU_FLOOR_HEIGHT)</pre> That is, no casting required. For testing purposes, we could temporarily (or permanently?) disable all int/fixed implicit conversions and make sure that those are happening only where they should be.
After beta4 is released, we can clean things up so that fixed_t is used instead of ints where appropriate.
I'm thinking of an arrangement where a (Python) script reads a set of map data declarations and outputs two header files: 1) the internal one with <tt>typedef struct line_s { ... } line_t</tt>, etc., 2) and a public one with property type definitions, e.g., the following:
<pre>#define DMPT_LINE_FLAGS DDVT_SHORT</pre>
Both the game and the engine could then use the DMPT defines to determine the DDVT type of a specific property. Internally, GetProperty and SetProperty would use these instead of propertyTypes.
The downside is that we then have a custom declaration syntax for the map data, but since the script is really simple it's not that troublesome. Python (and Perl, naturally) is a great tool for this kind of processing. (I prefer Python because of its cleaner syntax.) Since the map data types and properties change quite seldom, I don't think this adds much complexity. The good part is that both the map data structs and the type defines would only need to be stored in one place, and there would be no chance of mistaken types. Plus the types can be visible to the games as well, and everything is determined at compile-time (no dynamic overhead).
Ah yes, I was forgetting that. I implemented a quick "fix" in as much that at least the lightgrid is now queried but they don't yet react to bias light sources.
I was wondering about this, do you plan on doing per vertex lighting with models (bias light sources)?
<blockquote>Fortunately, int == fixed_t. So, the following should be fine:</blockquote>
Indeed. I'd like to see the "correct" type being used as it removes any potential confusion but as you say we can do that after beta4.
<blockquote>For testing purposes, we could temporarily (or permanently?) disable all int/fixed implicit conversions and make sure that those are happening only where they should be.</blockquote>
I'm happy with disabling the implicit conversions completely in this case int/fixed.
<blockquote>I'm thinking of an arrangement where a (Python) script reads a set of map data declarations and outputs two header files...</blockquote>
That sounds like the optimum solution to me.
Also, considering the map data structs are now handled via DMU/DAM (ie engine-side), this absolves the games of any responsibility. This has a side benefit in that Python is not a requirement in order to compile the games.
How do you plan on implementing the script?
Incidentally, from the little time I've spent with Python it does have much better syntax than Perl (I'm not exactly a Perl fan).
The propeties line->sidenum[2] pose a problem. The games currently use eg:
side = P_GetPtrp(line, DMU_SIDE0);
However line->sidenum[2] is an array of side indices and not side pointers so DMU performs the automatic conversion from side index to ptr (as DMU_SIDE0 is listed as a PTR in the properyTypes array).
When the automatic propertyTypes script is implemented - Doomsday will bomb on numerous counts of:
Get/SetValue: DDVT_INT incompatible with value type DDVT_PTR
as there is currently no automatic conversion from DDVT_INT to DDVT_PTR. Now, we could implement conversion from DDVT_INT to DDVT_PTR when the property IS an index but I'm fairly sure there will be other problems.
I'm wondering though, wouldn't we better off to change line->sidenum[2] to be an array of ptrs instead of indices? After all, pointers can be easily converted to indices (since we now store the object type inside the object's "header" we don't need to know the objects type to make the conversion eg P_ToIndex + DMU_GetType). To convert indices to ptrs requires the object type be specified seperately thus type checking should be implemented wherever indices are used as opposed to ptrs.
Naturally, the line->side pointers would need to be set as early as possible in DAM (while the indices are currently being read, since lines are now read AFTER sides this is possible to do this immediately).
Obviously we'd have to change all cases where these properties are referenced... but the changes should be straight forward.
<blockquote>Obviously we'd have to change all cases where these properties are referenced...</blockquote> We'd better avoid doing anything that requires any further major changes.
<blockquote>(P_GetPtr(line, DMU_SIDE0) should work without problems, as should P_GetInt(line, DMU_SIDE0).)</blockquote>
What I was trying to explain is that when the propertyTypes script is implemented - the above will not work without modification.
Currently, DMU_SIDE0 is declared as type DDVT_PTR in the propertyTypes array even though the data is not represented using pointers, rather int indices are used and the current DMU code relies on the automatic conversion of int to ptr.
When the propertyTypes script is implemented we'll be declaring line->sidenum[2] as an array of DDVT_INT.
Whilest there is not a problem currently, after the propertyTypes script is implemented there will be further changes needed.
For example P_GetPtr(line, DMU_SIDE0) will not return a ptr to side_t, it will instead return ptr to int. Currently there is no logic in place in DMU to distinguish between a data object indexe and a "regular" DDVT_INT, so we can't simply introduce type conversions (since conversion is determined based on the use of the property, as well as the property data type).
In the above case - what SHOULD Doomsday return? Logically one would argue that Doomsday should convert the side indexe to a ptr - that requires some changes in the way values are returned using Set/GetProperty and setargs.
Certainly new conversions can be introduced somewhere though (a new DDVT_INDICE data type identifier might be the best solution, with appropriate conversions).
These arn't big problems but are issues that we'll need to resolve.
After a little bit of testing it seems there are a few issues in XG after the update to DMU. I'll try and get these resolved this week as well as the minor bugs/glitches I keep putting off (menu cursor alignment and the automap level name among others).
DMT_LINE_SIDE0 is defined as DDVT_SHORT.
The fact that DMU_SIDE0 and DMU_SIDE1 are stored in the same array is not visible outside the engine.
P_GetPtr(line, DMU_SIDE0) returns a pointer to side_t, with DMU handling the conversion in GetProperty. Remember, P_GetPtr does not return pointers to properties, it returns the value of a property as a pointer (providing the value can be interpreted as or is a pointer). The current implementation should already work this way.
P_GetInt(line, DMU_SIDE0) returns the sidenum index directly. The current implementation should work: it converts the side_t pointer back to index, in GetValue.
I.e., GetProperty is free to check the return type and choose what to do. The DMT_* definitions that define the actual storage type of the properties, and don't really affect which P_Get function is used. We can have GetProperty do special handling with specific properties thanks to the large switch-cases, where each property is handled as an individual case.
When it comes to the definitions and the script, I wrote one already, but it needs to be slightly modified so that two header files are generated. Quite trivial stuff, though. It shouldn't take more than a few hours to actually write the data struct definitions once the script is working. However, I have no idea when I have the time... Hopefully sooner than later.
<blockquote>Remember, P_GetPtr does not return pointers to properties, it returns the value of a property as a pointer (providing the value can be interpreted as or is a pointer).</blockquote>Yes I'm sometimes forgetting that.
<blockquote>It shouldn't take more than a few hours to actually write the data struct definitions once the script is working. However, I have no idea when I have the time... Hopefully sooner than later.</blockquote>If you finish the script off I can write the definitions.
Load a map from a PWAD that does not contain BSP data. As long as Doomsday is configured correctly - the BSP data will be generated correctly and the level will load.
When attempting to restart the level Doomsday will currently bomb during R_GetUniqueLevelID, reporting (eg):
<blockquote>W_CheckNumForName: "MAP01" not found.</blockquote>
After a bit of investigation it would appear that for some reason the wrong data lump cache is being checked, for some reason if the PWAD does NOT contain BSP data for the loaded map. On the first attempt to load the map Doomsday is checking the primary lump cache for this lump, on the second attempt it is switching to the auxiliary lump cache.