-
Notifications
You must be signed in to change notification settings - Fork 180
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
GUIScript portraits and more #1876
base: master
Are you sure you want to change the base?
Conversation
ee004c5
to
5729a52
Compare
8736e6e
to
2b5a292
Compare
So how much testing did this receive? |
I see a Python error when right-clicking on a spell in mage's spellbook across the games, not on master:
|
Things I observed that are broken:
|
0e0422c
to
01c1889
Compare
8cd6292
to
accc0ba
Compare
af86cc5
to
7405018
Compare
@MarcelHB could you see what this MSVC syntax error is on about?
I'm assuming we are conflicting with some |
7405018
to
48e858e
Compare
The error says OPTIONAL is already bad, so try undefing that. |
Yeah sorry, got distracted by the other disastrous bugs like #1919. Will refrain from any more other changes. |
@@ -77,8 +77,8 @@ def OpenEnemyWindow(chargen=0): | |||
else: | |||
RaceWindow = GemRB.LoadWindow (16, "GUIREC") | |||
pc = GemRB.GameGetSelectedPCSingle () | |||
Class = GemRB.GetVar ("LUClass") + 1 | |||
LevelDiff = GemRB.GetVar ("LevelDiff") | |||
Class = GemRB.GetVar ("LUClass") or 0 + 1 |
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.
Cases like this need parens, otherwise the +1 will be applied only to the error case.
>>> None or 0 + 1
1
>>> 2 or 0 + 1
2
EDIT: appears to be the only case. :)
def GetControl(self, newID): | ||
return GetView(self, newID) | ||
def GetControl(self, cid): | ||
if cid is not None and cid >= 0: # FIXME: some GUIScript functions are still returning values of -1 instead on None |
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 suggest we print the stack trace here, so the negative providers will be easier to track down.
ScriptID spkID = SetSpeaker(spk); | ||
targetID = SetTarget(tgt); |
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.
These are not modified later (we don't need a backup), so it could just be:
ScriptID spkID = SetSpeaker(spk); | |
targetID = SetTarget(tgt); | |
SetSpeaker(spk); | |
SetTarget(tgt); |
(with the other spkID change reverted)
gemrb/core/DialogHandler.cpp
Outdated
@@ -404,8 +403,10 @@ bool DialogHandler::DialogChoose(unsigned int choose) | |||
EndDialog(); | |||
return false; | |||
} | |||
|
|||
auto targetID = core->GetDictionary().GetAs("DLG_TARGET_ID", 0); |
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.
Should be DLG_TARGET if anything. But since only DialogHandler is setting it, why do we need another lookup? targetID
should already match the var.
} | ||
// this is set when opening the store and not updated when changing PC | ||
// TODO: seems like it would be a good enhancement to set this to the selected PC with the highest charisma |
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.
Yeah, we can do much better — the games always used the party leader, whoever is in the first slot, regardless of who is talking.
SonarCloud Quality Gate failed. 1 Bug No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Brad, can you finish this soonish? It keeps getting conflicts against master, since it's a large body of work |
I'll try to find some time to dust it off this weekend. |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Do you need help, should we just postpone, something else? |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
This is the foundational pieces of #1788 along with some changes to address #1870
Checklist