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

Add ignore spawn block monster flag #3601

Merged
merged 4 commits into from
Aug 29, 2021
Merged

Conversation

marmichalski
Copy link
Contributor

@marmichalski marmichalski commented Aug 25, 2021

Pull Request Prelude

Changes Proposed

Issues addressed: #3600 (spawn block, black knight)
Supersedes: #2964

@marmichalski

This comment has been minimized.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

other than missing register_monster_type.lua entry, everything alright

@ghost

This comment has been minimized.

@marmichalski marmichalski force-pushed the spawn-block branch 4 times, most recently from 624fa9a to 8c58e40 Compare August 25, 2021 06:23
@ghost ghost requested review from DSpeichert, nekiro, ranisalt and yamaken93 August 25, 2021 07:25
nekiro
nekiro previously approved these changes Aug 25, 2021
@soul4soul
Copy link
Contributor

soul4soul commented Aug 25, 2021

otservbr already has this feature, they called the flag isBlockable. While it's not as descriptive as what you picked it would be nice to keep compatibility where possible.

Should the rest of the non-blockable creature flags be added in this commit too? A decent list is on tibiawiki https://tibia.fandom.com/wiki/Updates/10.70#Non-Blockable_Creatures

@marmichalski
Copy link
Contributor Author

otservbr already has this feature, they called the flag isBlockable. While it's not as descriptive as what you picked it would be nice to keep compatibility where possible.

It's hard to tell what that means. Can't you block it's path? Can't it block yours? Can't you block it's attacks?

About marking monster's spawns as non-blockable, I would not add it at all to the repo, as it's a breaking change (your spawns will act differently). Cip, as usual, made the spawns non-blockable by accident. Add one feature, break 5 others 🙄

@ghost
Copy link

ghost commented Aug 25, 2021

yeah we decided to add only the functionality, but not the monsters

@ranisalt
Copy link
Member

otservbr already has this feature, they called the flag isBlockable. While it's not as descriptive as what you picked it would be nice to keep compatibility where possible.

It seems perfectly reasonable to support it and issue a warning asking the user to rename before TFS 1.next

@marmichalski
Copy link
Contributor Author

otservbr already has this feature, they called the flag isBlockable. While it's not as descriptive as what you picked it would be nice to keep compatibility where possible.

It seems perfectly reasonable to support it and issue a warning asking the user to rename before TFS 1.next

I, on the other hand, see no reason to support something just because a downstream project had it implemented. 🤷

@Zbizu
Copy link
Contributor

Zbizu commented Aug 26, 2021

They can adjust their code to ours. It's just one string.

@ghost
Copy link

ghost commented Aug 26, 2021

I have not tested but I don't think their monster files is compatible with tfs anyway

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 see ignoresSpawnBlock and ignorespawnblock used interchangeably. Extremely confusing, let's settle on one spelling throughout.

@ghost
Copy link

ghost commented Aug 28, 2021

🤔 by the way can you add the delayed spawn feature together with this PR? #2964

@marmichalski
Copy link
Contributor Author

🤔 by the way can you add the delayed spawn feature together with this PR? #2964

No. I can do that in next PR.

@Erza
Copy link
Contributor

Erza commented Aug 28, 2021

I see ignoresSpawnBlock and ignorespawnblock used interchangeably. Extremely confusing, let's settle on one spelling throughout.

It's just in XML, like every other flag. All flags are lowercase in XML.

XML

<flag ignorespawnblock="0" />
<flag canpushitems="1" />
<flag canpushcreatures="1" />
<flag targetdistance="1" />
<flag staticattack="90" />
<flag runonhealth="0" />

C++

bool ignoresSpawnBlock = false;
bool canPushItems = false;
bool canPushCreatures = false;
int32_t targetDistance = 1;
uint32_t staticAttackChance = 95;
int32_t runAwayHealth = 0;

About the spellings it's even more different in other instances

staticattack -> staticAttackChance 
runonhealth -> runAwayHealth
summonable -> isSummonable
...

ignoresSpawnBlock is perfectly in line with how other methods are named

ignores...
is...
can...

Erza
Erza previously approved these changes Aug 28, 2021
@DSpeichert
Copy link
Member

ignoresSpawnBlock => ignores Spawn Block

ignorespawnblock => ignore spawn block

The missing s looks like a typo, and if it's not a typo, it's an unnecessary difference. I know other attributes differ in naming between XML and C++ but there's no reason for them to be different here other than to confuse the reader.

@marmichalski
Copy link
Contributor Author

Contributing to this project is worse than walking on shattered glass. Even when something gets approved, after wave of nonsense/offtopic/useless comments, it gets stalled forever. Truly the most demotivating project I have ever contributed to.

@joseluis2g
Copy link
Contributor

yeah because trying to correct a typo is the worst thing you can do

@marmichalski
Copy link
Contributor Author

yeah because trying to correct a typo is the worst thing you can do

This is not a typo and this is not about this SINGLE PR.

@nekiro
Copy link
Member

nekiro commented Aug 28, 2021

Contributing to this project is worse than walking on shattered glass. Even when something gets approved, after wave of nonsense/offtopic/useless comments, it gets stalled forever. Truly the most demotivating project I have ever contributed to.

Yep and your attitude isn't helping either.

@otland otland locked as too heated and limited conversation to collaborators Aug 28, 2021
ghost
ghost previously approved these changes Aug 29, 2021
data/scripts/lib/register_monster_type.lua Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
src/luascript.h Outdated Show resolved Hide resolved
src/monsters.cpp Outdated Show resolved Hide resolved
src/monsters.h Outdated Show resolved Hide resolved
src/spawn.cpp Outdated Show resolved Hide resolved
@ghost ghost dismissed stale reviews from Erza, nekiro, and themself via 963858c August 29, 2021 07:31
ghost
ghost previously approved these changes Aug 29, 2021
@ghost ghost requested a review from DSpeichert August 29, 2021 07:37
@marmichalski marmichalski force-pushed the spawn-block branch 3 times, most recently from ee3492d to f9c0802 Compare August 29, 2021 08:36
@marmichalski marmichalski requested review from a user and nekiro August 29, 2021 10:00
src/spawn.cpp Outdated Show resolved Hide resolved
@DSpeichert DSpeichert merged commit 634ac9f into otland:master Aug 29, 2021
@marmichalski marmichalski deleted the spawn-block branch September 3, 2021 16:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants