Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leads, leashed mobs and leash knots on fences #3798

Merged
merged 26 commits into from Aug 21, 2017
Merged

Leads, leashed mobs and leash knots on fences #3798

merged 26 commits into from Aug 21, 2017

Conversation

spekdrum
Copy link
Contributor

@spekdrum spekdrum commented Jun 21, 2017

  • Adds leads handling, leashed mobs and leash knots on fences.
  • Drops appropriate pickup on unleash.
  • Save and load knots from map and re-leash mobs that were leashed before server close.
  • Knots sound.
  • Support from 1.8 to 1.12 protocol versions.

It's been a lot of work and testing but that was very enjoyable :P

Fixes #2915 and #2729

Tests done:

  • Create some leads ( /i lead 24 ). They are stack-able now, as in vanilla.
  • Go for passive mobs (pigs, cows, horses...) and right click them with a lead on main hand. They will be leashed to you.
  • Try on others than passive mobs (wont work).
  • Take more than one mob at a time. You are now Santa Claus.
  • Look them following you.
  • Try to go far from the mob (> 10 blocks). Leads should break with no drop.
  • Unleash a leashed mob right clicking on it, then used lead will become a pickup.
  • Kill the mob while it's leashed to you, leash link (the drawn rope between you and the mob) will go out and there will be no drop.
  • Catch a mob with a lead and then right click on a fence, a leash knot will show up. The leash sound should be heard. The mob will unleash from your player and get leashed to the knot.
  • Try to leash more mobs to the same fence block. No more knots will be created, the existing will be reused. No sound will be reproduced.
  • Try to leash more mobs to the knot (it's different to the server right clicking on a knot than right click on the fence sustaining the knot). It will work.
  • Try to break a knot, all mobs leashed will be released.
  • Try to break a fence where there is a knot. As in vanilla, the knot will break passed about a second, and mobs will be released.
  • Try to leash a mob that's currently leashed to a fence leash knot. No effect and no rope line drawing. That last has been tricky to fix, because client anticipates to server and draws the line of the leash between player and the mob without regard from the current state of the mob.
  • Try to put several leash knots on adjacent fence blocks, logout, close server, open server again, login. Every mob will be leashed to it's knot again.

Not implemented:

  • Leashed mobs in vanilla can go around up to 5 blocks distance from the knot. Know mobs just go to the target pos like in other situations.
  • Surely I forgot some LUA and doc stuff

Copy link
Member

@madmaxoft madmaxoft left a comment

Choose a reason for hiding this comment

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

I think all mobs should be capable of being leashed, and each cMonster instance should have a flag saying whether that particular mob can be leashed - so that plugins may override it. Imagine a plugin for a tame creeper-on-a-leash :)

Also note that the PassiveMonster tick code has changed the assumptions somewhat - now the m_LoveTimer etc. can reach negative values, unlike in previous versions. This will most likely not matter if you move the logic out to cMonster, though.

Upon inspecting the ticking code, I think it will be prone to failures across chunk borders - when the entity is loaded and the knot is in a neighboring unloaded chunk. That would mean lost leashings (I have a feeling this happened in Vanilla, too), possibly even crashes.

Why is there even a timeout between destroying a fence and destroying the knot? Why not break the knot immediately? Makes much more sense.

Style issues:

  • multi-line if statements should have each condition on a separate line (including the first one) and the boolean operator at the end of the line:
if (
  (a == b) &&   // Possible comment on the reasons for this check
  !IsTicking()  // Why this condition?
)
{
  // ...
}
  • Member variables that store bools should be named with a yes/no verb in front: m_IsLeashed..., m_ShouldCheck... etc. (cLeashKnot::m_SelfDestroy)

@spekdrum
Copy link
Contributor Author

Agree with upgrading to cMonster, this will also make cleaner code.
I will think on a solution for the chunks problem, first I will try to reproduce it.

The delay for the knot is as in vanilla. I thought they did it this way to point out to the player he ruined a knot breaking the fence or whatever. But maybe vanilla behaves this way for some limitation that cuberite dont suffer :p

Im on holidays now, will put hands on this in a week or so. Happy coding ;)

@spekdrum
Copy link
Contributor Author

I'm working on the possibility to leash any monster and I found a problem. With any aggressive monster (tried creeper and skeleton) the client sends the right click event twice, when you just right click once on it. The result is that the monster gets leashed but immediately unleashed.
Should I have to use a variable to store last leash action tick count in order to avoid this? So ugly...

@Seadragon91
Copy link
Contributor

Seadragon91 commented Jun 28, 2017

Ah that problem. Had also been reported in this issue with sitting a wolf: #3186

Not sure whats going on there...

@spekdrum
Copy link
Contributor Author

Looking at the call stack, having put a breakpoint in cClientHandle::HandleUseEntity it seems that is the client that sends the packet twice...

@spekdrum
Copy link
Contributor Author

spekdrum commented Jun 28, 2017

I found an interesting point with the double right click problem.
This is the code that gets executed twice:

void cProtocol_1_9_0::HandlePacketUseEntity(cByteBuffer & a_ByteBuffer)
{
	HANDLE_READ(a_ByteBuffer, ReadVarInt, UInt32, EntityID);
	HANDLE_READ(a_ByteBuffer, ReadVarInt, UInt32, Type);

	switch (Type)
	{
		case 0:
		{
			HANDLE_READ(a_ByteBuffer, ReadVarInt, UInt32, Hand)
			m_Client->HandleUseEntity(EntityID, false);
			break;
		}

The "Hand" variable first time is 0 and second time is 1. According to the protocol doc (http://wiki.vg/Protocol#Use_Entity) that means main hand (0) or off-hand (1). Maybe we should check which hand holds the item we want to use with the entity, but we need to change the code a bit in order to let this parameter arrive to every place. I suggest to initially ignore calls for the off-hand and implement off-hand actions later. It's indeed a very easy fix. This way we can get rid of those double action related issues till a complete solution is done.

Edit: I've just make a PR for a fix.

@spekdrum
Copy link
Contributor Author

@madmaxoft As suggested I moved the logic to cMonster with a flag.
I've been doing some tests regarding the chunk borders potencial problem. I reproduced the problem, no crash happened, just mobs became unleashed and knot was still there (with no mob leashed). To fix that, I removed the line that sets m_LeashToPos to nullptr. This way the mob keeps trying till the knot is found. I introduced a stepped Tick there to avoid unnecessary calcs.

The delay for the knot destroy upon fence wipe is still there, because vanilla does this way. I like to remark the player the knot is gone. If you insist I have no problem to remove it, but please consider my argument :)

@spekdrum
Copy link
Contributor Author

I added an overload for cBoundingBox in order to get horizontal and vertical radius

@Seadragon91
Copy link
Contributor

Seadragon91 commented Jul 1, 2017

Does this work with monsters like creeper now?

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 1, 2017

Only if you override m_CanBeLeashed flag. By default only passive monsters have this flag on.
You can see it in cMonster constructor.

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 2, 2017

I fixed a possible leash lose when a mob is looking for it's knot and server gets closed. Also stopped mobs when they are leashed to a knot that has not been found yet.

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 2, 2017

Added speeding up mobs getting far from player when leashed to a player as Vanilla does.

@madmaxoft madmaxoft dismissed their stale review July 3, 2017 09:02

Code was rewritten

@@ -27,6 +27,7 @@ class cBoundingBox
cBoundingBox(double a_MinX, double a_MaxX, double a_MinY, double a_MaxY, double a_MinZ, double a_MaxZ);
cBoundingBox(const Vector3d & a_Min, const Vector3d & a_Max);
cBoundingBox(const Vector3d & a_Pos, double a_Radius, double a_Height);
cBoundingBox(const Vector3d & a_Pos, double a_Radius, double a_Height, double a_VerticalOffset);
Copy link
Member

Choose a reason for hiding this comment

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

This deserves a doxy-comment.

@@ -1704,6 +1704,7 @@ void cClientHandle::HandleUpdateSign(
void cClientHandle::HandleUseEntity(UInt32 a_TargetEntityID, bool a_IsLeftClick)
{
// TODO: Let plugins interfere via a hook
LOGD("Use entity %d", a_TargetEntityID);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be deleted.

else
{
// Detect if fence is destroyed. Get block at knot position and check if its solid
BLOCKTYPE BlockID = m_World->GetBlock(static_cast<int>(GetPosX()), static_cast<int>(GetPosY()), static_cast<int>(GetPosZ()));
Copy link
Member

Choose a reason for hiding this comment

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

Use FloorC instead of the casts.

@@ -131,24 +131,28 @@ void cHorse::OnRightClicked(cPlayer & a_Player)
}
else if (a_Player.GetEquippedItem().IsEmpty())
{
if (m_Attachee != nullptr)
// Check if leashed / unleashed to player before try to ride
if (!m_IsLeadActionJustDone)
Copy link
Member

Choose a reason for hiding this comment

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

Lead or Leash?

@@ -582,6 +685,27 @@ void cMonster::OnRightClicked(cPlayer & a_Player)
a_Player.GetInventory().RemoveOneEquippedItem();
}
}

// Using leads
m_IsLeadActionJustDone = false;
Copy link
Member

Choose a reason for hiding this comment

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

Leash

void cProtocol_1_12::SendLeashEntity(const cEntity & a_Entity, const cEntity & a_EntityLeashedTo)
{
ASSERT(m_State == 3); // In game mode?
cPacketizer Pkt(*this, 0x3C); // Set Attach Entity packet
Copy link
Member

Choose a reason for hiding this comment

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

Prefer lowercase hex digits

@@ -90,6 +90,7 @@ class cProtocol_1_8_0 :
virtual void SendHideTitle (void) override;
virtual void SendInventorySlot (char a_WindowID, short a_SlotNum, const cItem & a_Item) override;
virtual void SendKeepAlive (UInt32 a_PingID) override;
virtual void SendLeashEntity (const cEntity & a_Entity, const cEntity & a_EntityLeashedTo) override;
Copy link
Member

Choose a reason for hiding this comment

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

Wrong alignment, one more space

void cProtocol_1_9_0::SendLeashEntity(const cEntity & a_Entity, const cEntity & a_EntityLeashedTo)
{
ASSERT(m_State == 3); // In game mode?
cPacketizer Pkt(*this, 0x3A); // Set Attach Entity packet
Copy link
Member

Choose a reason for hiding this comment

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

lowercase hex

void cProtocol_1_9_0::SendUnleashEntity(const cEntity & a_Entity)
{
ASSERT(m_State == 3); // In game mode?
cPacketizer Pkt(*this, 0x3A); // Set Attach Entity packet
Copy link
Member

Choose a reason for hiding this comment

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

lowercase hex

{
LeashedToPos = a_Monster->GetLeashToPos();
}
else if (a_Monster->GetLeashedTo()->IsLeashKnot())
Copy link
Member

Choose a reason for hiding this comment

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

Could both LeashedToPos and LeashedTo be nullptr?

Copy link
Contributor Author

@spekdrum spekdrum Jul 4, 2017

Choose a reason for hiding this comment

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

No, because the outer if ensures that one of them (at least) is not null.
a_Monster->IsLeashed() it's the same as a_Monster->GetLeashedTo() != nullptr

@madmaxoft
Copy link
Member

This is definitely on the right way, just a few cosmetics now :)

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 4, 2017

Should I mark dirty a chunk when a player places a leash knot on a fence?

@Seadragon91
Copy link
Contributor

Seadragon91 commented Jul 5, 2017

Is a new entity created? If yes then it should be marked dirty

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 5, 2017

Ok, do not merge please, I'm going to mark them all dirty :P

@spekdrum
Copy link
Contributor Author

spekdrum commented Jul 5, 2017

I checked the code and there is no further change needed. MarkDirty() is already called later, once the entity is initialized and added to vector m_EntitiesToAdd from World. Same happens when destroyed. Everything happens in cWorld::Tick.

So, for me this is done!

virtual bool OnUse(cChunkInterface & a_ChunkInterface, cWorldInterface & a_WorldInterface, cPlayer * a_Player, int a_BlockX, int a_BlockY, int a_BlockZ, eBlockFace a_BlockFace, int a_CursorX, int a_CursorY, int a_CursorZ) override
{
// this vector allows to store the reference to an existing knot, if found inside the callback function
std::vector<cLeashKnot *> LeashKnotList;
Copy link
Member

Choose a reason for hiding this comment

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

Why a vector? A simple pointer should suffice for this.

{
super::OnRightClicked(a_Player);

// Check leashed nearby mobs to tight them to this knot
Copy link
Member

Choose a reason for hiding this comment

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

tight -> tie

return false;
}
} Callback(this, &a_Player);
a_Player.GetWorld()->ForEachEntityInBox(cBoundingBox(GetPosition(), 8, 8, -4), Callback);
Copy link
Member

Choose a reason for hiding this comment

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

This big callback code seems repeated in cBlockFenceHandler::OnUse. Could it be moved into a method of its own (TieHandLeashedMobs (?)) and shared?

if (!cBlockInfo::IsSolid(BlockID))
{
m_ShouldSelfDestroy = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to happen here? Couldn't the fence being broken signalize that the knot should be destroyed?

// TODO: leashed mobs in vanilla can move around up to 5 blocks distance from leash origin
MoveToPosition(m_LeashedTo->GetPosition());

// If distance to target > 10 break lead
Copy link
Member

Choose a reason for hiding this comment

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

lead -> leash

@@ -582,6 +685,27 @@ void cMonster::OnRightClicked(cPlayer & a_Player)
a_Player.GetInventory().RemoveOneEquippedItem();
}
}

// Using leads
Copy link
Member

Choose a reason for hiding this comment

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

leash




void cEntity::RemoveLeashedMob(cMonster * a_Monster, bool a_DropPickup, bool broadcast)
Copy link
Member

Choose a reason for hiding this comment

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

Style: a_ShouldBroadcast

@@ -2247,3 +2264,38 @@ void cEntity::SetPosition(const Vector3d & a_Position)




void cEntity::AddLeashedMob(cMonster * a_Monster, bool broadcast)
Copy link
Member

Choose a reason for hiding this comment

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

Style: a_ShouldBroadcast

// Leashed to a knot
bool Leashed;
int LeashedIdx = a_NBT.FindChildByName(a_TagIdx, "Leashed");
if (LeashedIdx >= 0)
Copy link
Member

Choose a reason for hiding this comment

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

Use an early bailout (negate the condition, return) to avoid unnecessary indent levels.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps even move this into a separate function, since LoadMonsterBase is already pretty long by itself.

@spekdrum
Copy link
Contributor Author

spekdrum commented Aug 5, 2017

Done

{
LeashedMob->Unleash(true, true, false);
}
m_LeashedMobs.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Can you try if this works? Removing the need for Unleash's last parameter.

while (!m_LeashedMobs.empty())
{
  m_LeashedMobs.front()->Unleash(true, true);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like, much better


cLeashKnot * cLeashKnot::FindKnotAtPos(cWorldInterface & a_WorldInterface, Vector3i a_BlockPos)
{
cLeashKnot * LeashKnot = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Remove this variable, make LookForKnot::m_LeashKnot a simple pointer and do return CallbackFindKnot.m_LeashKnot in the end.

void cMonster::CalcLeashActions()
{
// This mob just spotted in the world and [m_LeashToPos not null] shows that should be leashed to a leash knot at m_LeashToPos.
// This keeps trying until knot is found
Copy link
Member

Choose a reason for hiding this comment

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

Add a note that the leash knot may be in a different chunk that needn't be loaded, to make it clear why this can happen.

// Mob is already leashed but client anticipates the server action and draws a leash link, so we need to send current leash to cancel it
m_World->BroadcastLeashEntity(*this, *this->GetLeashedTo());
}
else if (CanBeLeashed() && (EquippedItem.m_ItemType == E_ITEM_LEAD))
Copy link
Member

Choose a reason for hiding this comment

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

How about adding E_ITEM_LEASH equivalent to the lead, but better named, and using that everywhere in code instead?


void cMonster::LeashTo(cEntity * a_Entity, bool a_ShouldBroadcast)
{
m_LeashedTo = a_Entity;
Copy link
Member

Choose a reason for hiding this comment

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

You should probably add ASSERT(m_LeashedTo == nullptr), so that when broken code in the future tries to call this while already leashed, the error is detected early and precisely.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, is this the directly exported Lua API? Plugins are bound to call this while already leashed, because we plugin authors are stupid and too lazy to read docs. So make the function behave well in such a case - ideally re-leash the mob to the new entity, or if not practically possible, don't change the leash-ness.

Copy link
Member

Choose a reason for hiding this comment

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

Similar with Unleash being called while not leashed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean check if already leashed and if so then do nothing, (and viceversa) right?

@spekdrum
Copy link
Contributor Author

spekdrum commented Aug 7, 2017

That's kinda poetic man, you are leashing me to this xD

@@ -459,6 +459,7 @@ enum ENUM_ITEM_ID : short
E_ITEM_GOLD_HORSE_ARMOR = 418,
E_ITEM_DIAMOND_HORSE_ARMOR = 419,
E_ITEM_LEAD = 420,
E_ITEM_LEASH = E_ITEM_LEAD,
Copy link
Contributor

@Bond-009 Bond-009 Aug 8, 2017

Choose a reason for hiding this comment

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

but-why
But, ... Why?

Copy link
Member

Choose a reason for hiding this comment

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

'cause I said so. Leash is a much better name than lead, because it's unambiguous. Lead can be also a metal, or a verb.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why not only use Leash?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the vanilla item is named minecraft:lead?

Copy link
Contributor Author

@spekdrum spekdrum Aug 9, 2017

Choose a reason for hiding this comment

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

Cause he/she said so 😄
Not a very important thing to argue about, others did it before 😛
https://english.stackexchange.com/questions/273617/difference-between-lead-and-leash

@spekdrum
Copy link
Contributor Author

spekdrum commented Aug 8, 2017

Done.
For LeashTo / Unleash I've just add code to check if already / not leashed and do nothing if so.

@madmaxoft
Copy link
Member

Yayyy, approved :)

@spekdrum
Copy link
Contributor Author

spekdrum commented Aug 9, 2017

crying-cat

@madmaxoft
Copy link
Member

Leash cat

@spekdrum
Copy link
Contributor Author

spekdrum commented Aug 9, 2017

LOL a leashed cat!

@madmaxoft
Copy link
Member

madmaxoft commented Aug 17, 2017

I'm afraid the recent merge of int-to-vector change in cBoundingBox has produced some merge conflicts (sorry). Can you fix those please?

@spekdrum
Copy link
Contributor Author

Sure

# Conflicts:
#	src/BoundingBox.cpp
#	src/BoundingBox.h
@spekdrum
Copy link
Contributor Author

I hope I did it well. I also had to change a bit of code affected by the entity ownership changes.

@madmaxoft madmaxoft merged commit b18f663 into master Aug 21, 2017
@madmaxoft
Copy link
Member

🎉

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.

Leads do not work properly
5 participants