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

Feature: RapidTransport, check for cooldown and handle payment #331

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Feature: RapidTransport, check for cooldown and handle payment #331

wants to merge 1 commit into from
+81 −9

Conversation

mifrgr
Copy link

@mifrgr mifrgr commented Oct 23, 2020 •
edited

Handles payment and and check for cooldown. Thanks to Kirmmin for his help! And all the reviewers, too!

MaxtorCoder
MaxtorCoder suggested changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@MaxtorCoder MaxtorCoder left a comment •
edited

Choose a reason for hiding this comment

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

Too many magic numbers, also rebase would be neato

Source/NexusForever.WorldServer/Game/Spell/SpellEffectHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Game/Spell/SpellEffectHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs Outdated Show resolved Hide resolved
@mifrgr mifrgr requested a review from kirmmin October 23, 2020 19:58
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch from 612064d to d816411 Compare October 24, 2020 22:49
@alex-anderson156
Copy link
Contributor

@mifrgr Did you forget to add [GameData] to the TaxiRoute table in the GameTableManager? without adding that i cannot get this to work.

@mifrgr
Copy link
Author

mifrgr commented Oct 27, 2020

@mifrgr Did you forget to add [GameData] to the TaxiRoute table in the GameTableManager? without adding that i cannot get this to work.

Yeah, I did, thanks, I will edit this when I'm at home

mifrgr
mifrgr commented Oct 27, 2020
View reviewed changes
Copy link
Author

@mifrgr mifrgr left a comment

Choose a reason for hiding this comment

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

All requested changes should be done now

xNxExOx
xNxExOx suggested changes Nov 17, 2020
View reviewed changes
Copy link

@xNxExOx xNxExOx left a comment

Choose a reason for hiding this comment

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

Validate before actions, not after them.

Source/NexusForever.WorldServer/Game/Spell/SpellEffectHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs Outdated Show resolved Hide resolved
Source/NexusForever.WorldServer/Network/Message/Handler/CharacterHandler.cs


//Check requirement for RapidTransport(ServiceToken)
if (cooldown > 0 && session.AccountCurrencyManager.CanAfford(AccountCurrencyType.ServiceToken, serviceTokenPrice))
Copy link

Choose a reason for hiding this comment

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

cooldown does not provide error message, is it intentional, because client should not be able to use it at when on cooldown, and informing "cheater" is not important? I think it is at least worth a comment.

Copy link
Author

Choose a reason for hiding this comment

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

The opcode that the client sends is the same regardless of whether the spell is on cooldown or not. So even if a cheater tried to get the RT for money, the server would still deduct the service tokens because it checks whether the spell is on cooldown. Cheating would be completely pointless in this case.

xNxExOx
xNxExOx suggested changes Nov 17, 2020
View reviewed changes
Copy link

@xNxExOx xNxExOx left a comment

Choose a reason for hiding this comment

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

validations should be before performing the action

Source/NexusForever.WorldServer/Game/Spell/SpellEffectHandler.cs Outdated Show resolved Hide resolved
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch 2 times, most recently from cc51904 to e291e4d Compare November 17, 2020 18:24
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch from f5bb7da to bda7aef Compare November 17, 2020 19:19
@mifrgr mifrgr changed the title Rapid transport cost + cooldown Feature: RapidTransport, check for cooldown and handle payment Nov 17, 2020
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch from bda7aef to a4fdaaf Compare November 19, 2020 19:22
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch 3 times, most recently from 8f09c5c to a82a0eb Compare February 5, 2021 19:23
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch from 2e9bc4e to 4581660 Compare February 5, 2021 20:02
@Rawaho
Copy link
Member

Rawaho commented Feb 8, 2021

From what I can see these checks should be handled in a prerequisite handler, id 269 is only used by spells 82922 and 82956.
Handling the checks in the spell effect has the problem that the spell has already cast by that point, we can't send the correct cast result when it fails due to lack of currency or invalid rapid transport node.
I'm going to extend the prerequisite manager to handle this correctly and do a bit of a rework.

@mifrgr
Implement Payment & Check for Cooldown
b23c247 
Added entry to load GameTable

Clean up

Add check for payment in the spell effect handler

delete space

Fix Build fail

Rename for alternate use

Rename after Check failed

Use .NET library instead of creation function

Delete space
@mifrgr mifrgr force-pushed the RapidTransport-Cost-+-Cooldown branch from 4581660 to b23c247 Compare July 3, 2021 21:26
@Rawaho Rawaho added this to the Pull Request Cleanup milestone Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-anderson156 alex-anderson156 alex-anderson156 approved these changes

@xNxExOx xNxExOx xNxExOx approved these changes

@MaxtorCoder MaxtorCoder MaxtorCoder approved these changes

@kirmmin kirmmin kirmmin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Pull Request Cleanup
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mifrgr @alex-anderson156 @Rawaho @xNxExOx @MaxtorCoder @kirmmin