Skip to content

[core] Refactor item pointer ownership#9870

Merged
WinterSolstice8 merged 2 commits into
LandSandBoat:basefrom
sruon:item_rewrite
Apr 28, 2026
Merged

[core] Refactor item pointer ownership#9870
WinterSolstice8 merged 2 commits into
LandSandBoat:basefrom
sruon:item_rewrite

Conversation

@sruon
Copy link
Copy Markdown
Contributor

@sruon sruon commented Apr 26, 2026

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Part 1 of several aimed at streamlining everything that's wrong with items.

  • Hides the template items from the global scope
  • Provides 3 public facing API in xi::items namespace:
    • lookup() -> Get a pointer to the const CItem template
    • spawn() -> Get a net new unique_ptr to copy of CItem template at given ID
    • clone() -> Get a net new unique_ptr to a copy of an arbitrary CItem
      • Callers responsible for storing or otherwise letting it go out of scope
  • Introduces xi::Badge for later usage

This overall does not change much but it:

  • Ensures we're not leaking items as much as we have in the past by forgetting to destroy an item passed through a dozen callers
  • Enforces a stronger distinction between read-only "templates" vs actual live items
  • Introduces xi::items namespace where subsequent lifecycle-handling will live

There's a handful of code smells left:

  • m_Weapons is still capturing raw pointers and destroying them
    • Need to rework this array entirely and separate PC/non-PC
  • dboxutils is still managing raw pointers
    • Need to rework the container first
  • There's a const_cast in CLuaItem because there are cases where we pass read-only items to the lua
    • May have to split CLuaItem into two classes later

Plan for next steps

Item state

Introduce an ItemState enum on CItem to replace the generic ITEM_LOCKED: Free, Equipped, Bazaar, PlacedFurniture, InTransaction (more on that below). The generic check in the codebase to know "can this item be used" will then be a simpler "item->isBusy()" as opposed to checking ITEM_LOCKED + m_reserve etc.

xi::items namespace will be the only place capable of shifting an item state to ensure competing systems aren't unlocking things they shouldn't.

Transactions

Rework the various containers into Transaction-derived classes: NPCTrade / PlayerTrade, Synthesis, ItemUse, DeliveryBox

xi::items namespace being the only place capable of shifting an item from inventory to any transaction and rejecting very loudly out-of-sequence attempts.

Declarative trade API for lua

Massively reduce the trade container surface exposed to the lua, prefer a single entrypoint mapping to NPCTrade TX described above.

No confirming trade by hand, no accepting/rejecting items, no items being locked for no reason. A single table that's all.

entity.declaredTrades =
{
    {  -- Multiple blocks supported
        match = -- anyOf / partial keywords supported for alternative patterns
        {
            -- gil supported here
            items =
            {
                { xi.item.GHELSBA_CHEST_KEY,    1 },
                { xi.item.PALBOROUGH_CHEST_KEY, 1 },
                { xi.item.GIDDEUS_CHEST_KEY,    1 },
            },
        },
        acceptIf = function(player)
            return not player:hasKeyItem(xi.ki.AIRSHIP_PASS_FOR_KAZHAM)
        end,

        event =
        {
            id = 301,
            onFinish = function(player)
                npcUtil.giveKeyItem(player, xi.ki.AIRSHIP_PASS_FOR_KAZHAM)
                return true -- true or no return -> Items gets swallowed
            end,
        },
    },
}

Steps to test these changes

Retest every single codepath...

@github-actions
Copy link
Copy Markdown

✨ Hello and thanks for the PR! ✨

🤖: This is a friendly automated reminder that the maintainers won't look at your PR until you've properly completed all of the checkboxes in the pre-filled template.

Comment thread src/common/types/badge.h
// Badge<T>
//
// Pass-key.
// Only T can construct a Badge<T>, so any method that takes one as a parameter is effectively private to T.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a Badge be copied or moved? Should probably declare those with our macros:


#define DISALLOW_COPY(TypeName)                    \
    TypeName(const TypeName&)            = delete; \
    TypeName& operator=(const TypeName&) = delete;

#define DISALLOW_MOVE(TypeName)               \
    TypeName(TypeName&&)            = delete; \
    TypeName& operator=(TypeName&&) = delete;

#define DISALLOW_COPY_AND_MOVE(TypeName) \
    DISALLOW_COPY(TypeName)              \
    DISALLOW_MOVE(TypeName)

Comment thread src/common/types/badge.h
friend T;
Badge() = default;
};
} // namespace xi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more spacing

namespace xi
{

template <typename T>
class Badge
{
    friend T;
    Badge() = default;
};

} // namespace xi

Comment thread src/common/types/badge.h
Comment on lines +32 to +35
class Badge
{
friend T;
Badge() = default;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd explicitly list that the contructor is private, to hammer home how this is working:

class Badge
{
private:
    friend T;
    Badge() = default;

Comment thread src/common/types/badge.h

// Badge<T>
//
// Pass-key.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to leave a link to this post; https://awesomekling.github.io/Serenity-C++-patterns-The-Badge/

Comment thread src/map/lua/lua_item.h
{
CItem* m_PLuaItem;
const CItem* m_readItem; // observer; always set
CItem* m_writeItem; // null when wrapping a read-only template
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@zach2good
Copy link
Copy Markdown
Contributor

zach2good commented Apr 26, 2026

When this comes around, would probably be nice to have an enum or flags instead of a raw bool

            onFinish = function(player)
                npcUtil.giveKeyItem(player, xi.ki.AIRSHIP_PASS_FOR_KAZHAM)
                return xi.tradeFinished.CONSUME_KEYITEM + xi.tradeFinished.CONSUME_ITEM
            end,

@sruon sruon force-pushed the item_rewrite branch 2 times, most recently from af5a0e1 to 99b9846 Compare April 27, 2026 05:52
@sruon sruon marked this pull request as ready for review April 27, 2026 06:19
@sruon
Copy link
Copy Markdown
Contributor Author

sruon commented Apr 27, 2026

Addressed comments, added a handful of tests, retested manually what couldn't be easily automated.
Ran xi_test through ASan.

@WinterSolstice8 WinterSolstice8 merged commit b6cdfbd into LandSandBoat:base Apr 28, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants