Repository navigation
contract: typed enums for the game values the API passes as raw numbers - #60
Conversation
gen_enum_names.py now gives every enumeration marked `/// @flags` constexpr |, &, ^, ~, |=, &=, ^= and HasFlag / HasAnyFlag in its own namespace (forwarding to the new utils/EnumFlags.h), so a flag set keeps its type instead of round-tripping through std::to_underlying. The contract gains enums, values taken from D2MOO and checked against it at compile time in the 1.14d backend: - CollisionFlag: Room::CollisionAt / GetCollision / GetCollisionFlat and CheckUnitCollision's mask; the pathfinder's grid and lookups use it, its private collision:: constants give way to BLOCKS_WALK. - ItemFlag (Unit::ItemFlags), Act (Unit::Act, 0-based; the binding adds 1), UnitFlagEx (Unit::FlagsEx), MonsterSpecType (Unit::SpecType). - CharFlag grows to D2MOO's full CLIENTSAVEFLAG_* set; GetCharFlags returns it. - CharacterClass gains Warlock (D2R 3.0); createCharacter still accepts only the 1.14d classes. - PartyState (Party::PartyState, was PartyFlag) and RosterFlag (TestPvpFlag's mask). - ControlState (Control::State / SetState), UiFlag (GetUIFlag), QuestFlag (GetQuestFlag). - PlayerMode / MonsterMode / ObjectMode / ItemMode document Unit.mode. - Preset units carry UnitType instead of a raw number. Room::Flags had no caller (neither the bindings nor the reference expose it) and is gone. Scripts see the same numbers: every binding converts back, the JS docs gain value tables for the new types. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ResurrectedTrader
left a comment
There was a problem hiding this comment.
We could also have convert::To which asserts that E is scalar type etc, and reduce the static casts
| return 0; | ||
| } | ||
| return d2common::UNITS_TestCollisionWithUnit(p1, p2, mask); | ||
| return d2common::UNITS_TestCollisionWithUnit(p1, p2, std::to_underlying(mask)); |
There was a problem hiding this comment.
Prototype could take enum
There was a problem hiding this comment.
Kept uint32_t at this boundary on purpose. CollisionFlag is 16 bits, while the game reads a full 32-bit argument slot, and under the Windows calling conventions the bits above a narrower argument are unspecified - typing the prototype as CollisionFlag could hand the game garbage in the upper half. The call converts with std::to_underlying instead. The other three imports you flagged are 32-bit enums, so those now take the enum (7a11de5).
| static_assert(std::to_underlying(QuestFlag::Started) == QFLAG_STARTED); | ||
| static_assert(std::to_underlying(QuestFlag::CompletedBefore) == QFLAG_COMPLETEDBEFORE); | ||
|
|
||
| int32_t GetQuestFlag(uint32_t quest, QuestFlag flag) { |
There was a problem hiding this comment.
Could there be a better return value based on what the runctikn does? Also prototype could take enum?
There was a problem hiding this comment.
Done in 7a11de5. The game function is a single-bit test (D2MOO's QUESTRECORD_GetQuestState returns BITMANIP_GetBitState(...) != 0), so GetQuestFlag now returns bool and the import takes QuestFlag. getQuest still returns 0 / 1 to scripts, as the reference returns a number.
| bool TestPvpFlag(const Unit& a, const Unit& b, uint32_t flag) { | ||
| return d2client::PLAYERLIST_CheckFlag(a.Id(), b.Id(), flag) != 0; | ||
| bool TestPvpFlag(const Unit& a, const Unit& b, RosterFlag flag) { | ||
| return d2client::PLAYERLIST_CheckFlag(a.Id(), b.Id(), std::to_underlying(flag)) != 0; |
There was a problem hiding this comment.
Prototype could take enum?
There was a problem hiding this comment.
Done in 7a11de5: PLAYERLIST_CheckFlag takes RosterFlag.
| static_assert(std::to_underlying(UiFlag::RecipeScroll) == UI_RECIPESCROLL); | ||
|
|
||
| bool GetUIFlag(UiFlag flag) { | ||
| return d2client::UI_GetVar(std::to_underlying(flag)) != 0; |
There was a problem hiding this comment.
Prototype could take enum?
There was a problem hiding this comment.
Done in 7a11de5: UI_GetVar takes UiFlag, and the backend's own callers moved from D2MOO's UI_* ids to UiFlag::Cube / Game / NpcShop (the values are asserted equal to D2MOO's).
| const uint16_t* p = grid->pCollisionMask; | ||
| for (uint32_t j = 0; j < height; ++j) { | ||
| out.emplace_back(p, p + width); | ||
| out.push_back(CopyCells(p, width)); |
There was a problem hiding this comment.
Is this a lot more expensive than what it was?
There was a problem hiding this comment.
Slightly: the first version zero-filled the vector before the memcpy, one extra pass over a room's cells (a few KB), with the same single allocation. 7a11de5 converts in one pass (span | views::transform | ranges::to<std::vector>), so it is back to one allocation and one copy, like the old range constructor.
| uint32_t GetCharFlags(); | ||
| CharFlag GetCharFlags(); | ||
| std::optional<uint8_t> IsLadder(); | ||
|
|
There was a problem hiding this comment.
IsLadder should probably return a boolean unless kolbot care about this being numeric somehow?
There was a problem hiding this comment.
I'd keep it numeric. me.ladder is the raw ladder flag byte - the reference does vp.setDouble(pData->ladderflag) (with a commented-out bool version next to it) - and kolbot compares it as a number in places (me.ladder === ladder, me.ladder > 0), so a bool would change what those see. This PR does not touch it; if we pin down the bits it could become its own flag enum later.
| struct PresetUnitInfo { | ||
| uint32_t type = 0; | ||
| UnitType type{}; | ||
| Position roomPos; |
There was a problem hiding this comment.
Done in 7a11de5: all three members are = {} now (Position already zeroed itself through its default member initialisers, but it is explicit here now).
| if (!g.rect.Contains(pos)) | ||
| return false; | ||
| return (g.GetUnchecked(pos) & (collision::BLOCK_WALK | collision::BLOCK_PLAYER)) == 0; | ||
| return !HasAnyFlag(g.GetUnchecked(pos), BLOCKS_WALK); |
There was a problem hiding this comment.
This seems to change value and not use enum
There was a problem hiding this comment.
The value is the same - BLOCKS_WALK was CollisionFlag::Wall | CollisionFlag::NoPlayer = 0x9, exactly the old BLOCK_WALK | BLOCK_PLAYER, and !HasAnyFlag(x, m) is the old (x & m) == 0. The name read too much like the old BLOCK_WALK (0x1) though, so 7a11de5 drops the constant and spells CollisionFlag::Wall | CollisionFlag::NoPlayer where it is tested.
|
|
||
| bool CollisionLookup::IsBlockedSlow(Point p) { | ||
| return ((collision::BLOCK_WALK | collision::BLOCK_PLAYER) & GetCross(p)) != 0; | ||
| return HasAnyFlag(GetCross(p), BLOCKS_WALK); |
There was a problem hiding this comment.
Again. Could use enums through out this file.
There was a problem hiding this comment.
Same commit: the file now uses the enum throughout - the grid, the windows, the lookups and every bit test are CollisionFlag, with no named wrapper constants left.
- The 1.14d imports UI_GetVar, PLAYERLIST_CheckFlag and QUESTRECORD_GetQuestFlag take UiFlag / RosterFlag / QuestFlag (all 32-bit, same as the argument slot the game reads); the backend's own UI_GetVar callers use UiFlag instead of D2MOO's UI_* ids. - GetQuestFlag returns bool: the game function is a single-bit test. getQuest still hands scripts 0 / 1. - PresetUnitInfo initialises every member explicitly. - The pathfinder names Wall | NoPlayer where it tests it instead of a BLOCKS_WALK constant. - The 1.14d collision cell copy converts in one pass rather than zero-filling and then copying. - convert::To<T> replaces ToInt32 / ToUint32 / ToDouble / ToBool and the static_casts around them: enums read through their underlying type, integers up to 32 bits through V8's ToInt32 / ToUint32 by signedness, wider integers and floating point through ToNumber. Every call converts the same script value to the same bits as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Added |
The game contract passed many closed value sets around as raw integers: collision cells, item and unit flags, char flags, unit modes, UI flag ids, quest flags, party state, control state, preset unit types. This gives them contract enums, with values taken from D2MOO and checked against it at compile time in the 1.14d backend, and gives flag sets real bit operators.
Flag operators
gen_enum_names.pynow emits, for every enumeration marked/// @flags(the marker the API docs already read, detected by the same rule),constexpr|,&,^,~,|=,&=,^=plusHasFlag(every bit) andHasAnyFlag(any bit) in the enum's own namespace, forwarding to the newutils/EnumFlags.h. A flag set keeps its type instead of round-tripping throughstd::to_underlying. Only scoped enums may be marked.CLAUDE.mddescribes it under "Enum names".Contract enums
CollisionFlag(D2MOO names, bits and masks):Room::CollisionAt/GetCollision/GetCollisionFlat,CheckUnitCollision's mask. The pathfinder's grid and lookups use it; its privatecollision::constants give way toBLOCKS_WALK.ItemFlag(Unit::ItemFlags),UnitFlagEx(Unit::FlagsEx),MonsterSpecType(Unit::SpecType),Act(Unit::Act, now 0-based; the binding adds 1).CharFlaggrows to D2MOO's fullCLIENTSAVEFLAG_*set;GetCharFlagsreturns it.CharacterClassgainsWarlock;createCharacterstill accepts only the 1.14d classes.PartyState(Party::PartyState, wasPartyFlag) andRosterFlag(TestPvpFlag's mask).ControlState(Control::State/SetState),UiFlag(GetUIFlag),QuestFlag(GetQuestFlag).PlayerMode/MonsterMode/ObjectMode/ItemModedocumentUnit.mode.UnitTypeinstead of a raw number.Room::Flagshad no caller (neither the bindings nor the reference expose it) and is gone.The 1.14d backend's own structs
D2WinControlStrc::dwStateandD2PresetUnitStrc::nUnitTypetake the enum types (same size and offsets); D2MOO itself is untouched. A few D2R-only values (ItemFlag's Chronicle bits,UnitFlagEx::SharedStash,Warlock) are included so the contract does not need to change again for them.Behaviour
Scripts see the same numbers: every binding converts back to the same value, defaults and error paths are unchanged, and the JS docs /
d2bsng.d.tsgain value tables for the new types (-1stays valid for presettype, raw values stay valid forControl.disabled). The one input that is now narrowed,checkCollision's mask, is cut to 16 bits, as the referenceJSGame.cppalready does and as D2Common's collision test only reads.Five independent reviews (collision and pathfinder, the flag operators and every rewritten bit test, JS-visible values, the 1.14d backend, the contract and services with every enum value against D2MOO) found no behaviour change.
Testing
js_tests(169/169),check-format,lint,gen_enum_names.py --check.gen_dts.py, andtsc --noEmiton the generatedd2bsng.d.ts.🤖 Generated with Claude Code