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

Quest moved to lua (Part 2) #4388

Merged
merged 7 commits into from
Apr 19, 2023
Merged

Quest moved to lua (Part 2) #4388

merged 7 commits into from
Apr 19, 2023

Conversation

MillhioreBT
Copy link
Contributor

@MillhioreBT MillhioreBT commented Apr 7, 2023

Pull Request Prelude

  • 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.

Changes Proposed

This is the new version of PR #4261, everything is neater and more consistent with namespaces as you suggested @ranisalt

The tracker now updates if the value is -1

Issues addressed: Nothing!

EPuncker
EPuncker previously approved these changes Apr 7, 2023
omarcopires
omarcopires previously approved these changes Apr 7, 2023
@dex-89
Copy link

dex-89 commented Apr 7, 2023

I have only one question, what is the purpose of transferring quests and other elements from the engine to lua?

@MillhioreBT
Copy link
Contributor Author

MillhioreBT commented Apr 7, 2023

I have only one question, what is the purpose of transferring quests and other elements from the engine to lua?

The reason for moving this to lua are several:

  • Quests in lua are versatile, you can create your own quests based on your own criteria and not just storages.
  • Being in lua we are freeing load on sources, you don't need the wonderful low level power of C++ for this nonsense.
  • The packet event manager in lua needs work and here we are giving you one.
  • Everything that can be handled in lua should be in lua and not in the sources.
  • Etc...

If you are concerned about compatibility, the PR is compatible with xml.

data/lib/core/player.lua Outdated Show resolved Hide resolved
@Zbizu
Copy link
Contributor

Zbizu commented Apr 12, 2023

setting storage value to -1 does not update the quest in tracker (actually the quest tracker is often ignoring status update when storage values are out of scope)

steps to reproduce:

  • configure mission as start value -1, end value 10
  • set value to -1 (player:setStorageValue(key, value))
  • set value to -2
  • set value to 0 (observed: when oldValue is out of scope, but newValue is within scope, tracker update is not sent)
  • set value to anything within scope (updates properly)
  • set value to anything out of scope (updates properly)
  • set value to anything within scope (does not update)
  • set value to anything within scope again (updates properly)

also sry for not reviewing everything at once, I caught a cold and I'm taking breaks frequently

data/lib/core/quests.lua Outdated Show resolved Hide resolved
src/luascript.cpp Outdated Show resolved Hide resolved
@omarcopires
Copy link
Contributor

I believe xml compatibility should not be enabled by default and the quests.xml file should be removed.

Co-authored-by: Zbizu <Zbizu@users.noreply.github.com>
Zbizu
Zbizu previously approved these changes Apr 13, 2023
Copy link
Contributor

@Zbizu Zbizu left a comment

Choose a reason for hiding this comment

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

looks good so far (local tests)
If you wait one week, I can post results from production also.

EPuncker
EPuncker previously approved these changes Apr 13, 2023
@omarcopires
Copy link
Contributor

omarcopires commented Apr 14, 2023

A warning is being printed to the console when starting the server:
[Warning - Events::load] Unknown player method: onUpdateStorage

You forgot to add below code in events.cpp:

} else if (methodName == "onUpdateStorage") {
	info.playerOnUpdateStorage = event;

I didn't find any other problems besides that.

@MillhioreBT MillhioreBT dismissed stale reviews from EPuncker and Zbizu via 57df2a2 April 19, 2023 17:20
Copy link
Contributor

@Zbizu Zbizu left a comment

Choose a reason for hiding this comment

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

I've ran it successfully on production for 2 days and haven't had any error reports or server issues related to it.

@EPuncker EPuncker merged commit d50730a into otland:master Apr 19, 2023
14 checks passed
@ranisalt ranisalt mentioned this pull request Apr 22, 2023
3 tasks
@MillhioreBT MillhioreBT deleted the quest_lua branch June 4, 2023 15:29
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

6 participants