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

Quests moved to Lua #4261

Closed
wants to merge 5 commits into from
Closed

Conversation

MillhioreBT
Copy link
Contributor

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

  • Moved the Quest and Mission system to Lua completely.
  • No compatibility with the old quests.xml file (If someone wants to do it in another PR, they can do it with the help of these methods included in this PR Add XML parsing bindings for Lua #4108)
  • The Quest Tracker of each player that is tracking a quest will be reset if you use /reload scripts or all (This is because we don't want ghost quests that have been removed, changed, or replaced.)
  • To make this possible, 4 new events have been created: (onStorageUpdate, onQuestTracker, onQuestLog, onQuestLine

If you have suggestions or find any bugs please let me know, I've tested it, but your revisions are always needed.

Issues addressed: Nothing!

Copy link
Member

@ranisalt ranisalt left a comment

Choose a reason for hiding this comment

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

This is great. Feels like moving forward.

data/scripts/quests/#example.lua Outdated Show resolved Hide resolved
@ranisalt ranisalt mentioned this pull request Dec 13, 2022
@DSpeichert
Copy link
Member

LGTM on dropping XML support here, I don't think it made sense to have it so I won't be pushing for backwards compatibility on this.

@MillhioreBT
Copy link
Contributor Author

MillhioreBT commented Jan 20, 2023

LGTM on dropping XML support here, I don't think it made sense to have it so I won't be pushing for backwards compatibility on this.

Here the example of the xml reader from lua for compatibility with previous versions.

load_questsxml.lua
local questXml = XMLDocument("data/XML/quests.xml")
local child = questXml:child("quests")
local firstChild = child:firstChild()

while firstChild do
	local missionNode = firstChild:firstChild()
	local missions = {}
	while missionNode do
		local mission = {
			name = missionNode:attribute("name"),
			storageId = missionNode:attribute("storageid"),
			startValue = missionNode:attribute("storagevalue"),
			endValue = missionNode:attribute("endvalue"),
			ignoreEndValue = missionNode:attribute("ignoreendvalue"),
			description = missionNode:attribute("description")
		}

		if not mission.description then
			local missionState = missionNode:firstChild()
			local description = {}
			while missionState do
				description[missionState:attribute("id")] = missionState:attribute("description")
				missionState = missionState:nextSibling()
			end

			mission.description = description
		end

		missions[#missions + 1] = mission
		missionNode = missionNode:nextSibling()
	end

	Game.createQuest(firstChild:attribute("name"), {
		storageId = firstChild:attribute("startstorageid"),
		storageValue = firstChild:attribute("startstoragevalue"),
		missions = missions
	}):register()

	firstChild = firstChild:nextSibling()
end

It's basically placed in some .lua file in the data\scripts folder and now we have backwards compatibility again, should I add it to this PR?

EPuncker
EPuncker previously approved these changes Jan 21, 2023
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.

lets go

ranisalt
ranisalt previously approved these changes Feb 20, 2023
@MillhioreBT MillhioreBT dismissed stale reviews from ranisalt and EPuncker via 121d31b March 27, 2023 01:07
@EPuncker
Copy link
Contributor

EPuncker commented Apr 5, 2023

needs rebase and will be merged

@omarcopires
Copy link
Contributor

Works fine for me:
image

msg:addByte(0xD0)
msg:addByte(1)

local trackedQuests, trackeds = TrackedQuests(self, missionsId)
Copy link
Member

Choose a reason for hiding this comment

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

TrackedQuests is a bad name, makes it look like you are constructing a new value, but it's just a function. getTrackedQuests maybe? And you don't need to return trackeds just use #trackedQuests

function Player.sendQuestLog(self)
local msg = NetworkMessage()
msg:addByte(0xF0)
local quests = Quest.getQuests(self)
Copy link
Member

Choose a reason for hiding this comment

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

getQuests does not return all quests so it should be clear about it. getPlayerQuests

Also, I think it should be in the Game or Player namespace, not Quest

Comment on lines +39 to +41
<event class="Player" method="onQuestTracker" enabled="1" />
<event class="Player" method="onQuestLog" enabled="1" />
<event class="Player" method="onQuestLine" enabled="1" />
Copy link
Member

Choose a reason for hiding this comment

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

I would change these event names to be more explicit about what they are reacting to:

Suggested change
<event class="Player" method="onQuestTracker" enabled="1" />
<event class="Player" method="onQuestLog" enabled="1" />
<event class="Player" method="onQuestLine" enabled="1" />
<event class="Player" method="onRequestQuestTracker" enabled="1" />
<event class="Player" method="onRequestQuestLog" enabled="1" />
<event class="Player" method="onRequestQuestLine" enabled="1" />

Or open instead of request. All other events are (or should be) in the format on<action><subject>

Also, onStorageUpdate -> onUpdateStorage. onInventoryUpdate is incorrect.


setmetatable(Quest, {__call = function(_, id) return Quest.getById(id) end})
setmetatable(Mission, {__call = function(_, id) return Mission.getById(id) end})
setmetatable(TrackedQuests, {__call = function(_, player, missionsId) return TrackedQuests.getByMissionsId(player, missionsId) end})
Copy link
Member

Choose a reason for hiding this comment

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

These belong to Game or Player, no need to create metatables. Game.getQuestById(id), Player.getTrackedQuests() then Quest.getMissionById(id)

} else if (methodName == "onQuestLog") {
info.playerOnQuestLog = event;
} else if (methodName == "onQuestLine") {
info.playerOnQuestLine = event;
Copy link
Member

Choose a reason for hiding this comment

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

I think these can be removed if you move the packet parsing to Lua :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah <3

@MillhioreBT
Copy link
Contributor Author

Closed for inconsistency, I will reopen it in another PR already refactored and applying the suggestions mentioned here

@MillhioreBT MillhioreBT closed this Apr 7, 2023
@MillhioreBT MillhioreBT mentioned this pull request Apr 7, 2023
3 tasks
@MillhioreBT MillhioreBT deleted the quest_lua_completed branch April 7, 2023 05:53
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.

5 participants