-
Notifications
You must be signed in to change notification settings - Fork 535
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
[lua] Correct isValidEntry logic to trade for registration #5615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, apollyon has multiple entrances for this
@@ -26,7 +26,7 @@ local content = Limbus:new({ | |||
}) | |||
|
|||
function content:isValidEntry(player, npc) | |||
return self.entryNpc == '_12i' or self.entryNpc == '_127' | |||
return npc and (npc:getName() == '_12i' or npc:getName() == '_127') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for _, npcName in self.entryNpcs do
if npc:getName == npcName then
return true
end
return false
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entryNpcs is a table, entryNpc on construct is now converted to a table, so this was a miss on my part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also set entryNpcs in the constructor to { '_12i, '_127' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely now that I think of it, you won't even need the entry function in this BCNM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so for both we just need to remove the isvalidentry override and defined entryNpcs in their constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so. Sorry for the mixed info as I was thinking through things, but yeah, that should work on its own. Added support for multiple entrances with Divine Might, so the same logic should work here I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, simpler logic and it works.
Really love the new battlefield content structure. Easy to module changes, cleaner format, more things in one place
Dismissed because I wrongly assumed the system didn't support multiple entrances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot more elegant
I affirm:
What does this pull request do?
Closes #5613 .
The
isValidEntry
logic was referencing the battlefieldentryNpc
value (which never explicitly gets set, it gets passed and used to build a table of one entry forentryNpcs
) instead of the traded npc's nameSteps to test these changes
Give yourself all the key items and items and trade the 4 chips to the swirling vortex