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.

Sign up for GitHub

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

Jump to bottom

[Rules] Update logic checks everywhere for FVNoDropFlag. #2179

Merged
merged 8 commits into from
Jul 30, 2022
Merged

[Rules] Update logic checks everywhere for FVNoDropFlag. #2179

merged 8 commits into from
Jul 30, 2022

Conversation

Quintinon
Copy link
Contributor

FVNoDropFlag == 0 is disabled
FVNoDropFlag == 1 is enabled for everyone
FVNoDropFlag == 2 is enabled for Admin() >= Character:MinStatusForNoDropExemptions

@Quintinon
Update logic checks everywhere for FVNoDropFlag.
0c10e84 
FVNoDropFlag == 0 is disabled
FVNoDropFlag == 1 is enabled for everyone
FVNoDropFlag == 2 is enabled for Admin() >= Character:MinStatusForNoDropExemptions
@Quintinon
Copy link
Contributor Author

I noticed some inconsistency in the FVNoDropFlag logic so I made some corrections. I have not had a chance to test these yet as I do not have my full dev environment setup yet, but the logic was simple enough that I wanted to get a pull request up.

@Kinglykrab
Copy link
Contributor

Have you had time to set up an environment to test this?

Kinglykrab
Kinglykrab reviewed May 20, 2022
View reviewed changes
zone/inventory.cpp Outdated Show resolved Hide resolved
Kinglykrab
Kinglykrab reviewed May 20, 2022
View reviewed changes
zone/inventory.cpp Outdated Show resolved Hide resolved
Kinglykrab
Kinglykrab reviewed May 20, 2022
View reviewed changes
zone/trading.cpp Outdated Show resolved Hide resolved
Kinglykrab
Kinglykrab reviewed May 20, 2022
View reviewed changes
world/client.cpp Outdated Show resolved Hide resolved
@Quintinon
Copy link
Contributor Author

I haven't had a chance to run this PR locally yet, will get to it today or this weekend.

@Quintinon
Copy link
Contributor Author

Quintinon commented May 20, 2022 •
edited
Loading

Tested World:FVNoDropFlag == 0, 1, 2 and Character:MinStatusForNoDropExemptions <, ==, > Admin()
All cases worked as expected for displaying the flag to the client and allowing trades.

@Kinglykrab
Copy link
Contributor

Tested World:FVNoDropFlag == 0, 1, 2 and Character:MinStatusForNoDropExemptions <, ==, > Admin() All cases worked as expected for displaying the flag to the client and allowing trades.

Beautiful, I'll let some other eyes peek at this, but looks good to me. Thanks for the contribution. 👍

@Quintinon Quintinon closed this Jun 1, 2022
@Quintinon Quintinon reopened this Jun 1, 2022
@Quintinon
Copy link
Contributor Author

Accidentally closed, I reopened it.

@Kinglykrab Kinglykrab changed the title Update logic checks everywhere for FVNoDropFlag. [Rules] Update logic checks everywhere for FVNoDropFlag. Jun 3, 2022
@Quintinon
Copy link
Contributor Author

I ran these changes on my personal server for over a month. I noticed that attuned items still were not tradable, but this fixed NO DROP items from being tradeable like it should. I'll put a separate PR up to fix being able to trade attuned items later.

Akkadius
Akkadius reviewed Jul 3, 2022
View reviewed changes
world/client.cpp Outdated Show resolved Hide resolved
@Akkadius
Copy link
Member

Akkadius commented Jul 3, 2022

Another note on readability, these checks could benefit from client helpers to make the code a bit more readable beyond the constants themselves - function helper(s) that describe the intent of what's being checked for

Akkadius
Akkadius requested changes Jul 10, 2022
View reviewed changes
Copy link
Member

@Akkadius Akkadius left a comment

Choose a reason for hiding this comment

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

Bumping for constant changes before merge

@Akkadius
Copy link
Member

@Quintinon we plan on getting this one sorted?

Quintinon
Quintinon commented Jul 16, 2022
View reviewed changes
zone/inventory.cpp Outdated Show resolved Hide resolved
@Akkadius Akkadius merged commit c68ff9b into EQEmu:master Jul 30, 2022
mackal pushed a commit that referenced this pull request Sep 3, 2022
@Quintinon @Akkadius @mackal
[Rules] Update logic checks everywhere for FVNoDropFlag. (#2179)
f4f402a 
* Update logic checks everywhere for FVNoDropFlag.

FVNoDropFlag == 0 is disabled
FVNoDropFlag == 1 is enabled for everyone
FVNoDropFlag == 2 is enabled for Admin() >= Character:MinStatusForNoDropExemptions

* Adding extra parenthesis to reduce ambiquity of order of operations for FVNoDropFlag checks

* Move FVNoDropFlag checks into a helper function in emu_constants.cpp and make an enum for the possible values.
Added console warning if setting is outside of allowed values.

* Move to client scoped helper method

Co-authored-by: Akkadius <akkadius1@gmail.com>
catapultam-habeo pushed a commit to catapultam-habeo/pyrelight-server that referenced this pull request Mar 27, 2023
@Quintinon @Akkadius @catapultam-habeo
[Rules] Update logic checks everywhere for FVNoDropFlag. (EQEmu#2179)
497e169 
* Update logic checks everywhere for FVNoDropFlag.

FVNoDropFlag == 0 is disabled
FVNoDropFlag == 1 is enabled for everyone
FVNoDropFlag == 2 is enabled for Admin() >= Character:MinStatusForNoDropExemptions

* Adding extra parenthesis to reduce ambiquity of order of operations for FVNoDropFlag checks

* Move FVNoDropFlag checks into a helper function in emu_constants.cpp and make an enum for the possible values.
Added console warning if setting is outside of allowed values.

* Move to client scoped helper method

Co-authored-by: Akkadius <akkadius1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akkadius Akkadius Akkadius approved these changes

@hgtw hgtw hgtw approved these changes

@Kinglykrab Kinglykrab Kinglykrab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Quintinon @Kinglykrab @Akkadius @hgtw