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

[Feature] Breakable mana shield #3848

Merged
merged 51 commits into from
Apr 6, 2023
Merged

Conversation

ArturKnopik
Copy link
Contributor

@ArturKnopik ArturKnopik commented Dec 14, 2021

New config

  • useBreakableManaShield: bool
  • manaShieldBarForAll: bool

New Lua function

  • player:setManaShieldBar: allows to use mana shield bar as something else (e.g. boss fight timer)

Modified lua script

  • data/spells/scripts/support/magic_shield.lua: give possibility to use old mana shield and breakable mana shield

image

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

src/player.cpp Outdated Show resolved Hide resolved
@EPuncker EPuncker requested a review from a team December 14, 2021 18:41
@EPuncker EPuncker added the feature New feature or functionality label Dec 14, 2021
@omarcopires
Copy link
Contributor

omarcopires commented Dec 15, 2021

I like the changes, but I would like a global config for the old and new PVP (after balancing), so we could only use one config for all future changes. Another thing is, maybe we could split this request in two, as the magic shield and the rooted condition have no connection. Do not take my comment as negative review, it's just my opinion, thank you very much for bringing the changes!

I ended up finding that you didn't add any spells that break the magic shield, nor did you add the potion system to retrieve it, is this going to be added in the future or has it been forgotten?

Copy link
Contributor

@EPuncker EPuncker left a comment

Choose a reason for hiding this comment

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

excellent! I would love to see the magic shield potion and the shield canceling spell tho

@ghost
Copy link

ghost commented Dec 15, 2021

locking player in place could be a cool gm feature, having root condition optional to support both old and new clients

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented Dec 15, 2021

spells and potion related to config settings
TODO: remove condition root, apply Zbizu changes

Copy link
Contributor

@EPuncker EPuncker left a comment

Choose a reason for hiding this comment

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

great! magic shield potion can be added at data/actions/scripts/potions.lua (but please don't move this file to revscriptsys for now 😄) just follow how buff potions where added there

data/scripts/spells/magic_shield.lua Outdated Show resolved Hide resolved
data/scripts/spells/cancel_magic_shield.lua Outdated Show resolved Hide resolved
@ArturKnopik ArturKnopik changed the title Feature: Breakable mana shield and condition root Feature: Breakable mana shield Dec 16, 2021
data/actions/actions.xml Outdated Show resolved Hide resolved
data/actions/actions.xml Outdated Show resolved Hide resolved
EPuncker
EPuncker previously approved these changes Dec 17, 2021
Copy link
Member

@nekiro nekiro left a comment

Choose a reason for hiding this comment

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

I would move the breakable mana values to breakable mana shield condition, it's serialized to database and loaded on login, so you have all the values ready without adding anything to database nor handling these values, same thing with calculcations in game.cpp, you can do them on condition and put whole logic in condition, for example manaCondition->onDamageTaken() and use this inside of game.cpp to reduce mana shield amount and call onConditionEnd inside of it when needed

src/condition.cpp Show resolved Hide resolved
if (!Condition::startCondition(creature)) {
return false;
}
if (auto player = creature->getPlayer()) {
Copy link
Member

Choose a reason for hiding this comment

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

Player* player


void ConditionManaShield::endCondition(Creature* creature)
{
if (auto player = creature->getPlayer()) {
Copy link
Member

Choose a reason for hiding this comment

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

Player* player


void ConditionManaShield::addCondition(Creature* creature, const Condition* addCondition)
{
if (auto player = creature->getPlayer()) {
Copy link
Member

Choose a reason for hiding this comment

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

Player* player

bool ret = Condition::setParam(param, value);

switch (param) {
case CONDITION_PARAM_MANASHIELD_BREAKABLE:
Copy link
Member

Choose a reason for hiding this comment

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

missing indendation

src/game.cpp Outdated
manaShield = 0;
}
}
if (targetPlayer->getMana() == 0 && manaShield > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

new line

src/game.cpp Outdated
targetPlayer->removeCondition(CONDITION_MANASHIELD_BREAKABLE);
}
} else {
// Old Mana Shield
Copy link
Member

Choose a reason for hiding this comment

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

normal mana shield?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old system that use all mana and cannot be broken when mana reach value 0

Copy link
Contributor

Choose a reason for hiding this comment

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

I think nekiro means to rename "old mana shield" to "normal mana shield"

src/game.cpp Show resolved Hide resolved
src/player.h Outdated
@@ -1280,6 +1296,8 @@ class Player final : public Creature, public Cylinder
uint32_t editListId = 0;
uint32_t mana = 0;
uint32_t manaMax = 0;
uint16_t manaShield = 0;
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 be probably calculated from the condition you added

@@ -1561,7 +1561,7 @@ void ProtocolGame::sendBasicData()
msg.addByte(spellId);
}

msg.addByte(0x00); // is magic shield active (bool)
msg.addByte(player->getVocation()->getMagicShield()); // is magic shield active (bool)
Copy link
Member

@nekiro nekiro Dec 18, 2021

Choose a reason for hiding this comment

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

again, check if player has condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw if a player changes vocation where is this updated?

MillhioreBT and others added 3 commits December 18, 2021 17:49
> Specialised Magic Levels: items will be able to receive a magic level modifier for specific elements, e.g. "fire magic level +2", which means only fire damage spells are boosted. Possible elements include Fire, Earth, Energy, Ice, Holy, Death and Healing.

https://tibia.fandom.com/wiki/Updates/12.70.10953
@ArturKnopik ArturKnopik requested review from nekiro and MillhioreBT and removed request for nekiro November 22, 2022 15:52
AKnopik PC added 3 commits April 4, 2023 17:22
actions.xml
configmanager.h
@ArturKnopik ArturKnopik changed the title Feature: Breakable mana shield [Feature] Breakable mana shield Apr 4, 2023
@EPuncker
Copy link
Contributor

EPuncker commented Apr 4, 2023

yo guys, lets test this and give feedback, lets merge this!

Copy link
Contributor

@EPuncker EPuncker left a comment

Choose a reason for hiding this comment

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

need to remove the iologindata changes and related changes

@omarcopires
Copy link
Contributor

yo guys, lets test this and give feedback, lets merge this!

everything worked great, except the utamo vita spell in revscript, it said "you have the wrong vocation to cast this spell", but once I moved it to XML it worked just fine

@krecikondexin
Copy link

krecikondexin commented Apr 5, 2023

@EPuncker i forgot revert fix for debug build after master merge, i will revert this changes + fix formatting
@omarcopires i will check it today

@omarcopires
Copy link
Contributor

omarcopires commented Apr 5, 2023

Looking at the current code, I noticed that there is compatibility with the old formula being maintained. However, in my opinion, maintaining this compatibility only increases the amount of code and may be unnecessary for those who do not wish to use the old formula. Additionally, several changes have been made to configuration parameters and spells since version 1.3, including cooldowns and item limits. Therefore, I believe it is important to question whether we really need to maintain this compatibility or if it would be better to simplify the code and remove the old formula, allowing us to focus only on the newer versions. What do you think.

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented Apr 5, 2023

@omarcopires string_view issue appear again - fixed
@EPuncker reverted string_view fix

@omarcopires for me old mana shield is viable for EVO and FUN ots, personally i developed 12+ OTS(FUN) with old mana shield

@omarcopires
Copy link
Contributor

omarcopires commented Apr 5, 2023

I performed tests in several scenarios and did not encounter any issues within the game. The shield and icon were added and broken as expected, and the "utamo vita" spell now functions correctly. It is worth noting that the tests were only performed with the "druid" class.

image

@EPuncker
Copy link
Contributor

EPuncker commented Apr 5, 2023

there are 5 failing checks, can you verify them please?

@krecikondexin
Copy link

Screenshot from 2023-04-06 14-24-33
Fails not related to my changes.

@EPuncker EPuncker merged commit 87ca1d6 into otland:master Apr 6, 2023
@ArturKnopik ArturKnopik deleted the newManaShield branch April 10, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants