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

fix: update client on advance #4604

Merged
merged 5 commits into from
Dec 28, 2023
Merged

fix: update client on advance #4604

merged 5 commits into from
Dec 28, 2023

Conversation

omarcopires
Copy link
Contributor

Pull Request Prelude

Changes Proposed

Now, the screenshot (takeScreenshot) is only triggered if the new level is greater than the old level.

Issues addressed:
The client takes a screenshot even when the player loses level.

EPuncker
EPuncker previously approved these changes Dec 18, 2023
@@ -0,0 +1,17 @@
local updateClientOnAdvance = CreatureEvent("Update Client On Advance")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this file completely

Copy link
Contributor

@amatria amatria Dec 23, 2023

Choose a reason for hiding this comment

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

Also the new file name and title have less semantic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the script does exactly what the name says and now works as it should, there is no point in changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not at all. And please do not resolve my conversation if there is an open discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh great you hid my reply

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're not satisfied with my approach, make a request yourself and let the moderators decide.
You are being very annoying to someone who never or almost never contributes anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,17 @@
local updateClientOnAdvance = CreatureEvent("Update Client On Advance")

function updateClientOnAdvance.onAdvance(player, skill, oldLevel, newLevel)
Copy link
Contributor

@amatria amatria Dec 23, 2023

Choose a reason for hiding this comment

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

Not at all. This script is not doing "exactly what the name say" because then the script should be named "update_client_exp_display_on_level_change_and_take_screenshot_on_any_skill_change.lua"

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, no reason to remove the other two files.

Copy link
Contributor

@amatria amatria Dec 23, 2023

Choose a reason for hiding this comment

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

This new script has less semantic and more responsibilities.

Copy link
Contributor

@amatria amatria Dec 23, 2023

Choose a reason for hiding this comment

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

And please, do not resolve my comments if there is an ongoing discussion.

@EPuncker EPuncker merged commit 80f2398 into otland:master Dec 28, 2023
4 checks passed
@omarcopires omarcopires deleted the fix-screenshot branch December 28, 2023 03:36
@@ -6,7 +6,10 @@ function updateClientOnAdvanceLevel.onAdvance(player, skill, oldLevel, newLevel)
end

player:updateClientExpDisplay()
player:takeScreenshot(SCREENSHOT_TYPE_LEVELUP)

if newLevel > oldLevel then
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better. TYVM

@@ -1,7 +1,7 @@
local updateClientOnAdvanceSkill = CreatureEvent("Update Client On Advance Skill")

function updateClientOnAdvanceSkill.onAdvance(player, skill, oldLevel, newLevel)
if skill == SKILL_LEVEL then
if skill == SKILL_LEVEL and newLevel > oldLevel then
Copy link
Contributor

Choose a reason for hiding this comment

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

Though, why the and newLevel > oldLevel here? This script will now take a screenshot if the player losses a level.

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.

None yet

4 participants