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

persistent tracking of item affectance #857

Merged
6 commits merged into from
Jul 13, 2018
Merged

persistent tracking of item affectance #857

6 commits merged into from
Jul 13, 2018

Conversation

fartwhif
Copy link
Collaborator

@fartwhif fartwhif commented Jul 8, 2018

item affectance is restored at Player.EnterWorld

item affectance is restored at Player.EnterWorld
gmriggs
gmriggs previously approved these changes Jul 9, 2018
@gmriggs
Copy link
Collaborator

gmriggs commented Jul 9, 2018

Tested, functionality worked as expected

Before patching, I logged in with a character who had a magical item already equipped, and verified that the mana was not being depleted (pre-patch)

Still pre-patch, I dequipped and re-equipped the sword, and verified that the mana was indeed being depleted after re-equipping.

I also noticed that equipping magic items instantly used 1 mana upon wielding, not sure if that is intended / matches retail?

After patching, I logged in with the character while they had the magic sword already equipped, and the mana started ticking properly.

The spells for the wielded item also appeared in the player's enchantments properly

The 'IsAffecting' nomenclanture seems a bit non-standard to me (IsActivated?) but that is just a personal preference

Nice job with this patch, it really makes things feel more consistent!

@fartwhif
Copy link
Collaborator Author

fartwhif commented Jul 9, 2018

Thanks for the testing and critique! If I remember correctly retail did have items burn 1 point of mana immediately upon equipping (and successfully activating) it. I actually went and changed ACE to match that pattern (mana stone PR). Hopefully the "affecting" nomenclature doesn't collide with any concepts in the questing systems. maybe it was for questing, and "hot" or something else is actually for this concept, we will see. I came up with "activated" in the mana stone PR because there is no property with that name and I needed something non-persistent and unique to hold out until the persistence PR. "affecting" is part of the WorldObject property lists which is why I chose it for persistence, a guess that it's the correct one as there are a few others that have plausibly compatible names.

gmriggs
gmriggs previously approved these changes Jul 9, 2018
added null check for emote tells
fixed bug causing crash when using the /buff command
gmriggs
gmriggs previously approved these changes Jul 10, 2018
…sent to the client and in turn causing inconsistent behaviors in the client enchantment list and player stats
Copy link
Collaborator

@gmriggs gmriggs left a comment

Choose a reason for hiding this comment

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

Confirmed duplicate enchantment bug has been fixed!

@ghost ghost merged commit 9350622 into ACEmulator:master Jul 13, 2018
@fartwhif fartwhif deleted the persistAffecting branch December 7, 2018 02:14
This pull request was closed.
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.

None yet

2 participants