Hey everyone! This week I finally got back to the task given by Sev in my first week \u2014 converting all the raw struct casts in the Dungeon Master engine to portable byte-offset macros. This was responsible for undefined behavior that could cause crashes or wrong results on platforms with different endianness or alignment requirements.<\/p>\n
Earlier it used to be like this:<\/p>\n
\/\/ Struct definition\r\nclass TextString {\r\n Thing _nextThing;\r\n uint16 _textDataRef;\r\npublic:\r\n explicit TextString(uint16 *rawDat) : _nextThing(rawDat[0]), _textDataRef(rawDat[1]) {}\r\n bool isVisible() { return _textDataRef & 1; }\r\n void setVisible(bool visible) { _textDataRef = (_textDataRef & ~1) | (visible ? 1 : 0); }\r\n};<\/pre>\nNow it looks like this:<\/p>\n
#define TEXTSTRING_nextThing(address) (Thing(READ_LE_UINT16((address) + 0)))\r\n#define TEXTSTRING_textDataRef(address) READ_LE_UINT16((address) + 2)\r\n#define TEXTSTRING_isVisible(address) (READ_LE_UINT16((address) + 2) & 1)\r\n#define TEXTSTRING_setVisible(address, visible) WRITE_LE_UINT16((address) + 2, (READ_LE_UINT16((address) + 2) & ~1) | ((visible) ? 1 : 0))<\/pre>\nAnd the call site changed from:<\/p>\n
TextString textString(_thingData[kDMstringTypeText] + thing.getIndex() * _thingDataWordCount[kDMstringTypeText]);\r\nif (textString.isVisible()) { ... }<\/pre>\nTo:<\/p>\n
byte *textString = getThingData(thing);\r\nif (TEXTSTRING_isVisible(textString)) { ... }<\/pre>\nWhy this refactor was necessary<\/h4>\n
This refactor was necessary because the original code relied on casting raw game data directly to C++ structs, which assumes specific object layouts, alignment, and byte ordering. These assumptions are not guaranteed across different architectures, especially on big-endian machines. By switching to explicit byte offsets and endian-aware accessors, the engine now accesses game data in a portable and predictable way.<\/p>\n
Lesson 1:<\/h4>\n
It was a massive refactor across 13 structs in the engine and required multiple attempts. My first approach was still broken \u2014 I wrote:<\/p>\n
uint16 getTextDataRef() const { return READ_LE_UINT16((const uint16 *)this + 1); }<\/pre>\nI thought using READ_LE_UINT16<\/code> alone would fix the portability issue, but I missed the real problem: the C++ standard does not guarantee that a struct’s members are packed without padding. A compiler is free to insert padding between members, so the second field is not guaranteed to be located exactly one uint16<\/code> after the start of the object. As a result, (const uint16 *)this + 1<\/code> may not point to the second field at all. READ_LE_UINT16<\/code> handles endianness correctly, but it can’t save you if you’re reading from the wrong offset in the first place.<\/p>\nLesson 2:<\/h4>\n
After converting all the struct methods to macros operating on byte *<\/code>, I still had an issue at the call sites. I was passing non-byte *<\/code> pointers directly to the macros, relying on the cast inside the macro definition<\/strong> to handle it. While this compiled and ran, Sev pointed out that casts hide problems \u2014 you are forcing the compiler to interpret memory as a different type, relying on assumptions about object layout that may not hold on all platforms. By switching the call sites to use byte *<\/code> explicitly, the macros now operate directly on raw data using fixed byte offsets rather than through potentially misleading type casts.<\/p>\nResult<\/h4>\n
In total, this refactor touched 13 classes throughout the Dungeon Master engine. After several iterations, the final result is a portable implementation that accesses serialized game data explicitly through byte offsets.<\/p>\n
Another Freeze<\/h4>\n
Towards the end of the week I continued my playthrough of the game and reached level 9 out of the 13 dungeon levels. During testing I encountered another freeze when interacting with a Wizard Eye monster. After some debugging, I found that the issue originated from projectile allocation. Wizard Eyes attack by spawning projectile objects, which are allocated through getUnusedThing()<\/code>. However, the size of projectile data was being incorrectly divided by two using >> 1<\/code>, causing the engine to allocate only 2 words instead of the required 5. In ScummVM, _thingDataWordCount<\/code> already stores sizes in words, making the division unnecessary.
\nBefore:<\/p>\nint16 thingDataByteCount = _thingDataWordCount[thingType] >> 1;<\/pre>\nAfter:<\/p>\n
int16 thingDataWordCount = _thingDataWordCount[thingType];<\/pre>\nAs a result, newly spawned projectiles overlapped existing objects in memory and corrupted their linked-list pointers. In the failing case, a projectile’s next pointer was overwritten and linked back to Door 0<\/code> instead of terminating at _thingEndOfList<\/code>, creating a cyclic linked list on that map square. The rendering function drawObjectsCreaturesProjectilesExplosions()<\/code> then looped infinitely while traversing this list, freezing the game. Removing the obsolete >> 1<\/code> division restored the correct allocation size and resolved the issue.<\/p>\n <\/p>\n
That was it for this week. Next week I plan to finish my gameplay testing of Dungeon Master, continue hunting for any remaining issues, and start implementing the remaining unimplemented methods in the engine. See you in the next one. Until then, goodbye! \ud83d\ude42<\/p>\n","protected":false},"excerpt":{"rendered":"
Hey everyone! This week I finally got back to the task given by Sev in my first week \u2014 converting all the raw struct casts in the Dungeon Master engine to portable byte-offset macros. This was responsible for undefined behavior that could cause crashes or wrong results on platforms with different endianness or alignment requirements. […]<\/p>\n","protected":false},"author":30,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[8],"tags":[],"class_list":["post-158","post","type-post","status-publish","format-standard","hentry","category-week-4"],"_links":{"self":[{"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/posts\/158","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/users\/30"}],"replies":[{"embeddable":true,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/comments?post=158"}],"version-history":[{"count":62,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/posts\/158\/revisions"}],"predecessor-version":[{"id":226,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/posts\/158\/revisions\/226"}],"wp:attachment":[{"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/media?parent=158"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/categories?post=158"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blogs.scummvm.org\/mohit\/wp-json\/wp\/v2\/tags?post=158"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}