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

Remove condition by Condition parameters (fixes #3544) #3545

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

Erza
Copy link
Contributor

@Erza Erza commented Aug 7, 2021

Pull Request Prelude

Changes Proposed

A copy of the Condition object was added to the creature in LuaScriptInterface::luaCreatureAddCondition, therefore Creature::removeCondition could not remove the condition properly, as the Condition object that was constructed from Userdata did not match the one stored in the creature's condition list.

Also applied const correctness.

Issues addressed: closes #3544

A copy of the Condition object was added to the creature in `LuaScriptInterface::luaCreatureAddCondition`, therefore `Creature::removeCondition` could not remove the condition properly, as the Condition object that was constructed from Userdata did not match the one stored in the creature's condition list.

Also applied const correctness.
@EPuncker EPuncker requested a review from a team August 7, 2021 18:28
@MillhioreBT
Copy link
Contributor

If you use logic, you will realize that this PR does not make any sense

This function has two behaviors:

  • Remove by userdata: this gives us to understand that we need to pass the correct user data stored in the player conditions, if you pass a random or self-generated condition then it is very obvious that it will not remove anything since you random condition is not a player condition

  • Remove by type: this is the behavior of a lifetime, you specify the search parameters, and if these parameters match any condition stored in the player then we obtain this condition and eliminate it

¿How to use the function correctly with userdata?

creature:removeCondition(creature:getCondition(...), ...)

If you still insist, you can add an overload in lua and it looks much better, although counterintuitive and can lead people to think incorrectly, it is already out of basic logic.

@Erza
Copy link
Contributor Author

Erza commented Aug 7, 2021

If you use logic, you will realize that this PR does not make any sense

This function has two behaviors:

  • Remove by userdata: this gives us to understand that we need to pass the correct user data stored in the player conditions, if you pass a random or self-generated condition then it is very obvious that it will not remove anything since you random condition is not a player condition
  • Remove by type: this is the behavior of a lifetime, you specify the search parameters, and if these parameters match any condition stored in the player then we obtain this condition and eliminate it

¿How to use the function correctly with userdata?

creature:removeCondition(creature:getCondition(...), ...)

If you still insist, you can add an overload in lua and it looks much better, although counterintuitive and can lead people to think incorrectly, it is already out of basic logic.

Read #3544 (comment)

@MillhioreBT
Copy link
Contributor

MillhioreBT commented Aug 7, 2021

If you use logic, you will realize that this PR does not make any sense
This function has two behaviors:

  • Remove by userdata: this gives us to understand that we need to pass the correct user data stored in the player conditions, if you pass a random or self-generated condition then it is very obvious that it will not remove anything since you random condition is not a player condition
  • Remove by type: this is the behavior of a lifetime, you specify the search parameters, and if these parameters match any condition stored in the player then we obtain this condition and eliminate it

¿How to use the function correctly with userdata?

creature:removeCondition(creature:getCondition(...), ...)

If you still insist, you can add an overload in lua and it looks much better, although counterintuitive and can lead people to think incorrectly, it is already out of basic logic.

Read #3544 (comment)

And I understand what you mean by using a condition and then extracting its properties and then finding the player's condition and so bla bla bla
I thought about this first than you, before this feature was added, but then I realized that it is just an additional complication
For example, what happens if we pass the correct user data? in this case we have to look for it again and it is like a pretty silly code

In case you are too lazy to write 20 more letters you can do a help function:
data/lib/core/condition.lua

function Condition:unpack()
	return self:getType(), self:getId(), self:getSubId()
end

script.lua

-- force: default
creature:removeCondition(condition:unpack())
-- force: false
creature:removeCondition(condition:unpack(), false)
-- force: true
creature:removeCondition(condition:unpack(), true)

Copy link
Member

@DSpeichert DSpeichert left a comment

Choose a reason for hiding this comment

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

I'm not entirely against it, although we may be requiring in other places the Lua developer to also pass actual condition, not a replica. This behavior does not hurt though.

@DSpeichert DSpeichert changed the title Fix #3544 Remove condition by Condition parameters (fixes #3544) Feb 6, 2022
@Codinablack
Copy link
Contributor

All checks passing, all reviews passing. LGTM

PR is from 2021, its amazing its still able to be merged... Can we do so now?

@MillhioreBT MillhioreBT merged commit 3762d17 into otland:master Mar 19, 2024
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.

creature:removeCondition doesn't work properly with userdata
5 participants