Fixed bug preventing interaction with no item in-hand (#164)

BrentDaMage · web-flow · commit 1981a3c2804b · 2025-06-17T22:40:38.000-05:00
* Fixed bug preventing interaction with no item in-hand
* Cleaned up ItemInstance null handling
* Fixed Player::isUsingItem() logic

---------

Co-authored-by: Brent Da Mage &lt;BrentDaMage@users.noreply.github.com&gt;
diff --git a/source/client/app/Minecraft.cpp b/source/client/app/Minecraft.cpp
@@ -270,7 +270,8 @@ void Minecraft::handleBuildAction(const BuildActionIntention& action)
 {
 	LocalPlayer* player = m_pLocalPlayer;
 	bool canInteract = getTimeMs() - m_lastInteractTime >= 200;
-	if (player->isUsingItem()) return;
+	// This logic is present in 0.9.0, but just does not make any sense being here.
+	//if (player->isUsingItem()) return;
 
 	if (action.isDestroyStart() || action.isAttack())
 	{
@@ -350,13 +351,7 @@ void Minecraft::handleBuildAction(const BuildActionIntention& action)
 		else if (action.isPlace() && canInteract)
 		{
 			ItemInstance* pItem = getSelectedItem();
-			if (pItem &&
-				m_pGameMode->useItemOn(
-					player,
-					m_pLevel,
-					pItem->m_itemID <= 0 ? nullptr : pItem,
-					m_hitResult.m_tilePos,
-					m_hitResult.m_hitSide))
+			if (m_pGameMode->useItemOn(player, m_pLevel, pItem, m_hitResult.m_tilePos, m_hitResult.m_hitSide))
 			{
 				bInteract = false;
 
@@ -366,7 +361,7 @@ void Minecraft::handleBuildAction(const BuildActionIntention& action)
 
 				if (isOnline())
 				{
-					if (pItem->m_itemID > C_MAX_TILES || pItem->m_itemID < 0)
+					if (ItemInstance::isNull(pItem) || pItem->m_itemID > C_MAX_TILES)
 						return;
 
 					TilePos tp(m_hitResult.m_tilePos);
@@ -480,7 +475,7 @@ void Minecraft::tickInput()
 			else if (getOptions()->isKey(KM_DROP, keyCode))
 			{
 				ItemInstance *item = m_pLocalPlayer->m_pInventory->getSelected();
-				if (item != nullptr)
+				if (!ItemInstance::isNull(item))
 				{
 					ItemInstance itemDrop = m_pLocalPlayer->isSurvival() ? item->remove(1) : ItemInstance(*item);
 					itemDrop.m_count = 1;
@@ -1181,22 +1176,19 @@ ItemInstance* Minecraft::getSelectedItem()
 {
 	ItemInstance* pInst = m_pLocalPlayer->getSelectedItem();
 
-	if (!pInst)
+	if (ItemInstance::isNull(pInst))
 		return nullptr;
 
-	if (m_pGameMode->isSurvivalType())
-		return pInst;
-
-    // Create new "unlimited" ItemInstance for Creative mode
-    
-	if (pInst->isNull())
-		return nullptr;
-
-	m_CurrItemInstance.m_itemID = pInst->m_itemID;
-	m_CurrItemInstance.m_count = 999;
-	m_CurrItemInstance.setAuxValue(pInst->getAuxValue());
+	if (m_pGameMode->isCreativeType())
+	{
+		// Create new "unlimited" ItemInstance for Creative mode
+		m_CurrItemInstance.m_itemID = pInst->m_itemID;
+		m_CurrItemInstance.m_count = 999;
+		m_CurrItemInstance.setAuxValue(pInst->getAuxValue());
+		return &m_CurrItemInstance;
+	}
 
-	return &m_CurrItemInstance;
+	return pInst;
 }
 
 int Minecraft::getFpsIntlCounter()
diff --git a/source/client/gui/Gui.cpp b/source/client/gui/Gui.cpp
@@ -417,7 +417,7 @@ void Gui::renderSlot(int slot, int x, int y, float f)
 	Inventory* pInv = m_pMinecraft->m_pLocalPlayer->m_pInventory;
 
 	ItemInstance* pInst = pInv->getQuickSlotItem(slot);
-	if (pInst == nullptr || pInst->m_itemID <= 0)
+	if (ItemInstance::isNull(pInst))
 		return;
 
     float var6 = ((float)pInst->m_popTime) - f;
@@ -442,10 +442,7 @@ void Gui::renderSlotOverlay(int slot, int x, int y, float f)
 	Inventory* pInv = m_pMinecraft->m_pLocalPlayer->m_pInventory;
 
 	ItemInstance* pInst = pInv->getQuickSlotItem(slot);
-	if (!pInst)
-		return;
-
-	if (!pInst->m_itemID)
+	if (ItemInstance::isNull(pInst))
 		return;
 
 	ItemRenderer::renderGuiItemOverlay(m_pMinecraft->m_pFont, m_pMinecraft->m_pTextures, pInst, x, y);
diff --git a/source/client/gui/screens/IngameBlockSelectionScreen.cpp b/source/client/gui/screens/IngameBlockSelectionScreen.cpp
@@ -99,10 +99,7 @@ void IngameBlockSelectionScreen::init()
 void IngameBlockSelectionScreen::renderSlot(int index, int x, int y, float f)
 {
 	ItemInstance* pItem = getInventory()->getItem(index);
-	if (!pItem)
-		return;
-
-	if (!pItem->m_itemID)
+	if (ItemInstance::isNull(pItem))
 		return;
 
 	ItemRenderer::renderGuiItem(m_pMinecraft->m_pFont, m_pMinecraft->m_pTextures, pItem, x, y, true);
diff --git a/source/client/player/input/MouseBuildInput.cpp b/source/client/player/input/MouseBuildInput.cpp
@@ -9,6 +9,13 @@ MouseBuildInput::MouseBuildInput()
         m_lastButtonStates[i] = false;
 }
 
+void MouseBuildInput::_updateLastButtonStates()
+{
+    m_lastButtonStates[BUTTON_LEFT]   = Mouse::isButtonDown(BUTTON_LEFT);
+    m_lastButtonStates[BUTTON_RIGHT]  = Mouse::isButtonDown(BUTTON_RIGHT);
+    m_lastButtonStates[BUTTON_MIDDLE] = Mouse::isButtonDown(BUTTON_MIDDLE);
+}
+
 bool MouseBuildInput::tickBuild(Player* player, BuildActionIntention* buildActionIntention)
 {
     bool wroteIntention = false;
@@ -61,10 +68,7 @@ bool MouseBuildInput::tickBuild(Player* player, BuildActionIntention* buildActio
         *buildActionIntention = BuildActionIntention(intent);
     }
 
-    // Log last button states
-    m_lastButtonStates[BUTTON_LEFT] = Mouse::isButtonDown(BUTTON_LEFT);
-    m_lastButtonStates[BUTTON_RIGHT] = Mouse::isButtonDown(BUTTON_RIGHT);
-    m_lastButtonStates[BUTTON_MIDDLE] = Mouse::isButtonDown(BUTTON_MIDDLE);
+    _updateLastButtonStates();
 
     return wroteIntention;
 }
diff --git a/source/client/player/input/MouseBuildInput.hpp b/source/client/player/input/MouseBuildInput.hpp
@@ -12,6 +12,10 @@ class MouseBuildInput : public IBuildInput
 public:
 	MouseBuildInput();
 
+private:
+	void _updateLastButtonStates();
+
+public:
 	virtual bool tickBuild(Player* player, BuildActionIntention* buildActionIntention) override;
 };
 
diff --git a/source/client/player/input/UnifiedTurnBuild.cpp b/source/client/player/input/UnifiedTurnBuild.cpp
@@ -182,7 +182,8 @@ bool UnifiedTurnBuild::tickBuild(Player* pPlayer, BuildActionIntention* pIntenti
 			intent = BuildActionIntention::INTERACT;
 			wroteIntention = true;
 		}
-		else */if (field_24 /* && pPlayer->isUsingItem()*/) // Holds off on acknowledging interact intent until the user is absolutely sure a tick later
+		// Holds off on acknowledging interact intent until the user is absolutely sure a tick later
+		else */if (field_24 /*&& pPlayer->isUsingItem()*/) // Adding pPlayer->isUsingItem() makes player break blocks way too fast when not holding items
 		{
 			intent = BuildActionIntention::TOUCH_HOLD_CONTINUE;
 			wroteIntention = true;
diff --git a/source/client/renderer/ItemInHandRenderer.cpp b/source/client/renderer/ItemInHandRenderer.cpp
@@ -41,7 +41,7 @@ void ItemInHandRenderer::itemUsed()
 void ItemInHandRenderer::renderItem(ItemInstance* inst)
 {
 #ifndef ORIGINAL_CODE
-    if (inst->isNull())
+    if (ItemInstance::isNull(inst))
         return;
 #endif
     
@@ -197,7 +197,7 @@ void ItemInHandRenderer::render(float f)
     float fAnim = pLP->getAttackAnim(f);
     constexpr float d = 0.8f;
     
-	if (!pItem->isNull())
+	if (!ItemInstance::isNull(pItem))
 	{
         glTranslatef(-0.4f * Mth::sin(float(M_PI) * Mth::sqrt(fAnim)), 0.2f * Mth::sin(2.0f * float(M_PI) * Mth::sqrt(fAnim)), -0.2f * Mth::sin(float(M_PI) * fAnim));
         glTranslatef(0.7f * d, -0.65f * d - (1.0f - h) * 0.6f, -0.9f * d);
diff --git a/source/world/entity/Player.cpp b/source/world/entity/Player.cpp
@@ -250,7 +250,7 @@ void Player::displayClientMessage(const std::string& msg)
 
 void Player::drop(const ItemInstance* pItemInstance, bool b)
 {
-	if (pItemInstance->isNull())
+	if (ItemInstance::isNull(pItemInstance))
 		return;
 
 	ItemEntity* pItemEntity = new ItemEntity(m_pLevel, Vec3(m_pos.x, m_pos.y - 0.3f + getHeadHeight(), m_pos.z), pItemInstance);
diff --git a/source/world/entity/Player.hpp b/source/world/entity/Player.hpp
@@ -72,7 +72,7 @@ class Player : public Mob
 	bool isSurvival() const { return getPlayerGameType() == GAME_TYPE_SURVIVAL; }
 	bool isCreative() const { return getPlayerGameType() == GAME_TYPE_CREATIVE; }
 	ItemInstance* getSelectedItem() const;
-	bool isUsingItem() const { return false && !getSelectedItem()->isNull(); }
+	bool isUsingItem() const { return !ItemInstance::isNull(getSelectedItem()); }
 
 	// QUIRK: Yes, I did mean it like that, as did Mojang.
 #pragma GCC diagnostic push
diff --git a/source/world/item/Inventory.cpp b/source/world/item/Inventory.cpp
@@ -226,7 +226,7 @@ int Inventory::getQuickSlotItemId(int slotNo)
 	
 	int idx = m_hotbar[slotNo];
 	ItemInstance* pInst = getItem(idx);
-	if (!pInst)
+	if (ItemInstance::isNull(pInst))
 		return -1;
 
 	return pInst->m_itemID;
@@ -238,13 +238,8 @@ ItemInstance* Inventory::getQuickSlotItem(int slotNo)
 		return nullptr;
 	
 	ItemInstance* pInst = getItem(m_hotbar[slotNo]);
-	if (!pInst)
-		return nullptr;
-
-	if (pInst->m_itemID == 0)
-		return nullptr;
 
-	return pInst;
+	return !ItemInstance::isNull(pInst) ? pInst : nullptr;
 }
 
 ItemInstance* Inventory::getSelectedItem()
@@ -324,7 +319,7 @@ void Inventory::selectItemById(int itemID, int maxHotBarSlot)
 int Inventory::getAttackDamage(Entity* pEnt)
 {
 	ItemInstance* pInst = getSelected();
-	if (!pInst)
+	if (ItemInstance::isNull(pInst))
 		return 1;
 
 	return pInst->getAttackDamage(pEnt);
diff --git a/source/world/item/ItemInstance.cpp b/source/world/item/ItemInstance.cpp
@@ -151,29 +151,6 @@ bool ItemInstance::isStackedByData()
 	return getItem()->isStackedByData();
 }
 
-bool ItemInstance::matches(ItemInstance* other) const
-{
-	return this->getAuxValue() == other->getAuxValue() &&
-           this->m_count == other->m_count &&
-           this->m_itemID == other->m_itemID;
-}
-
-bool ItemInstance::matches(ItemInstance* a1, ItemInstance* a2)
-{
-	if (a1 == a2 && a1 == nullptr)
-		return true;
-
-	if (a1 == nullptr || a2 == nullptr)
-		return false;
-
-	return a1->matches(a2);
-}
-
-int ItemInstance::getAttackDamage(Entity *pEnt)
-{
-	return getItem()->getAttackDamage(pEnt);
-}
-
 void ItemInstance::mineBlock(const TilePos& pos, Facing::Name face)
 {
 	return getItem()->mineBlock(this, pos, face);
@@ -212,18 +189,54 @@ bool ItemInstance::useOn(Player* player, Level* level, const TilePos& pos, Facin
 	return getItem()->useOn(this, player, level, pos, face);
 }
 
+int ItemInstance::getAttackDamage(Entity* pEnt)
+{
+	return getItem()->getAttackDamage(pEnt);
+}
+
 bool ItemInstance::isNull() const
 {
 	// 0.9.2
 	if (m_itemID <= 0) // m_field_10, assuming this is m_itemID
 		return true;
 
-	if (m_auxValue != 0)
-		return false;
-	if (m_count != 0)
-		return false;
-	if (m_popTime != 0)
+	if (m_auxValue != 0 ||
+		m_count    != 0 ||
+		m_popTime  != 0)
+	{
 		return false;
+	}
 
 	return true; // isNull
 }
+
+bool ItemInstance::isNull(const ItemInstance* item)
+{
+	return item == nullptr || item->isNull();
+}
+
+bool ItemInstance::matches(const ItemInstance* a1, const ItemInstance* a2)
+{
+	if (a1 == a2 && a1 == nullptr)
+		return true;
+
+	if (a1 == nullptr || a2 == nullptr)
+		return false;
+
+	return a1 == a2;
+}
+
+bool ItemInstance::operator==(const ItemInstance& other) const
+{
+	return this->getAuxValue() == other.getAuxValue() &&
+		   this->m_count       == other.m_count &&
+		   this->m_itemID      == other.m_itemID;
+}
+
+bool ItemInstance::operator!=(const ItemInstance& other) const
+{
+	// doing this is likely more efficient than inverting the result of == after the fact
+	return this->getAuxValue() != other.getAuxValue() ||
+		   this->m_count       != other.m_count ||
+		   this->m_itemID      != other.m_itemID;
+}
diff --git a/source/world/item/ItemInstance.hpp b/source/world/item/ItemInstance.hpp
@@ -52,7 +52,6 @@ class ItemInstance
 	bool isDamaged();
 	bool isStackable();
 	bool isStackedByData();
-	bool matches(ItemInstance*) const;
 	void mineBlock(const TilePos& pos, Facing::Name face);
 	ItemInstance remove(int amt);
 	void setDescriptionId(const std::string&);
@@ -64,12 +63,17 @@ class ItemInstance
 	Item* getItem() const;
 	ItemInstance* copy();
 
-	static bool matches(ItemInstance*, ItemInstance*);
-
 	// v0.2.0
 	int getAttackDamage(Entity *pEnt);
 	bool isNull() const;
 
+	// @NOTE: Won't this be ambiguous with the non-static method?
+	static bool isNull(const ItemInstance*);
+	static bool matches(const ItemInstance*, const ItemInstance*);
+
+	bool operator==(const ItemInstance&) const;
+	bool operator!=(const ItemInstance&) const;
+
 public:
 	int m_count;
 	int m_popTime;

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,13 @@ MouseBuildInput::MouseBuildInput()`
`9`	`9`	`m_lastButtonStates[i] = false;`
`10`	`10`	`}`
`11`	`11`
	`12`	`+void MouseBuildInput::_updateLastButtonStates()`
	`13`	`+{`
	`14`	`+ m_lastButtonStates[BUTTON_LEFT] = Mouse::isButtonDown(BUTTON_LEFT);`
	`15`	`+ m_lastButtonStates[BUTTON_RIGHT] = Mouse::isButtonDown(BUTTON_RIGHT);`
	`16`	`+ m_lastButtonStates[BUTTON_MIDDLE] = Mouse::isButtonDown(BUTTON_MIDDLE);`
	`17`	`+}`
	`18`	`+`
`12`	`19`	`bool MouseBuildInput::tickBuild(Player* player, BuildActionIntention* buildActionIntention)`
`13`	`20`	`{`
`14`	`21`	`bool wroteIntention = false;`
`@@ -61,10 +68,7 @@ bool MouseBuildInput::tickBuild(Player* player, BuildActionIntention* buildActio`
`61`	`68`	`*buildActionIntention = BuildActionIntention(intent);`
`62`	`69`	`}`
`63`	`70`
`64`		`- // Log last button states`
`65`		`- m_lastButtonStates[BUTTON_LEFT] = Mouse::isButtonDown(BUTTON_LEFT);`
`66`		`- m_lastButtonStates[BUTTON_RIGHT] = Mouse::isButtonDown(BUTTON_RIGHT);`
`67`		`- m_lastButtonStates[BUTTON_MIDDLE] = Mouse::isButtonDown(BUTTON_MIDDLE);`
	`71`	`+ _updateLastButtonStates();`
`68`	`72`
`69`	`73`	`return wroteIntention;`
`70`	`74`	`}`
Original file line number	Diff line number	Diff line change
`@@ -182,7 +182,8 @@ bool UnifiedTurnBuild::tickBuild(Player* pPlayer, BuildActionIntention* pIntenti`
`182`	`182`	`intent = BuildActionIntention::INTERACT;`
`183`	`183`	`wroteIntention = true;`
`184`	`184`	`}`
`185`		`- else /if (field_24 / && pPlayer->isUsingItem()*/) // Holds off on acknowledging interact intent until the user is absolutely sure a tick later`
	`185`	`+ // Holds off on acknowledging interact intent until the user is absolutely sure a tick later`
	`186`	`+ else /if (field_24 /&& pPlayer->isUsingItem()*/) // Adding pPlayer->isUsingItem() makes player break blocks way too fast when not holding items`
`186`	`187`	`{`
`187`	`188`	`intent = BuildActionIntention::TOUCH_HOLD_CONTINUE;`
`188`	`189`	`wroteIntention = true;`
Original file line number	Diff line number	Diff line change
`@@ -250,7 +250,7 @@ void Player::displayClientMessage(const std::string& msg)`
`250`	`250`
`251`	`251`	`void Player::drop(const ItemInstance* pItemInstance, bool b)`
`252`	`252`	`{`
`253`		`- if (pItemInstance->isNull())`
	`253`	`+ if (ItemInstance::isNull(pItemInstance))`
`254`	`254`	`return;`
`255`	`255`
`256`	`256`	`ItemEntity* pItemEntity = new ItemEntity(m_pLevel, Vec3(m_pos.x, m_pos.y - 0.3f + getHeadHeight(), m_pos.z), pItemInstance);`