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

GUIScript portraits and more #1876

Open
wants to merge 100 commits into
base: master
Choose a base branch
from
Open

GUIScript portraits and more #1876

wants to merge 100 commits into from

Conversation

bradallred
Copy link
Member

@bradallred bradallred commented Aug 4, 2023

This is the foundational pieces of #1788 along with some changes to address #1870

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • I used the same coding style as the surrounding code
  • I have tested the proposed changes
  • I extended the documentation, if necessary
  • The proposed change builds also on our build bots (check after submission)

@bradallred bradallred force-pushed the PST-portraits branch 2 times, most recently from ee004c5 to 5729a52 Compare August 4, 2023 15:11
@lynxlynxlynx lynxlynxlynx linked an issue Aug 4, 2023 that may be closed by this pull request
@bradallred bradallred force-pushed the PST-portraits branch 2 times, most recently from 8736e6e to 2b5a292 Compare August 4, 2023 19:21
@lynxlynxlynx
Copy link
Member

So how much testing did this receive?

@MarcelHB
Copy link
Collaborator

MarcelHB commented Aug 4, 2023

I see a Python error when right-clicking on a spell in mage's spellbook across the games, not on master:

[Python/ERROR]: Traceback (most recent call last):
  File "D:/Sources\gemrb\gemrb\GUIScripts/GUIMG.py", line 263, in OpenMageSpellInfoWindow
    ResRef = MageKnownSpellList[index - 100]
             ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
IndexError: list index out of range

@MarcelHB
Copy link
Collaborator

MarcelHB commented Aug 4, 2023

Things I observed that are broken:

  • Commiting a Level-Up in BG2 doesn't work with some Python error
  • Interacting with items in the inventory, like drinking a potion over the button, doesn't work, no Python error around.

@bradallred bradallred marked this pull request as draft August 4, 2023 21:42
@bradallred bradallred assigned bradallred and unassigned bradallred Aug 4, 2023
@bradallred bradallred force-pushed the PST-portraits branch 3 times, most recently from 0e0422c to 01c1889 Compare August 8, 2023 03:25
@bradallred bradallred force-pushed the PST-portraits branch 2 times, most recently from 8cd6292 to accc0ba Compare August 14, 2023 20:05
gemrb/core/EnumFlags.h Outdated Show resolved Hide resolved
@bradallred bradallred force-pushed the PST-portraits branch 3 times, most recently from af86cc5 to 7405018 Compare August 15, 2023 00:50
@bradallred
Copy link
Member Author

@MarcelHB could you see what this MSVC syntax error is on about?

C:\projects\gemrb\gemrb\core\Store.h(46,49): error C2059: syntax error: '='

I'm assuming we are conflicting with some #define or some such.

@lynxlynxlynx
Copy link
Member

The error says OPTIONAL is already bad, so try undefing that.

@bradallred
Copy link
Member Author

I just rebased on master due to conflicts. Are we still just waiting for you to do some testing @lynxlynxlynx?

@lynxlynxlynx
Copy link
Member

Yeah sorry, got distracted by the other disastrous bugs like #1919. Will refrain from any more other changes.

@lynxlynxlynx
Copy link
Member

lynxlynxlynx commented Oct 15, 2023

One thing that I immediately noticed is that the action bar buttons are not always drawn on load — until you reselect a character.

  • The demo inventory errors out.
  • The demo (world) map window complains, but works.
  • pst hp labels are not drawn
  • pst change of animated portrait is delayed (not sure if new)
  • pst morte can cycle to two empty item descriptions in the inventory (not sure if new)
  • pst if you open the message window via the button, you can't close it via the close button (or its hotkey or esc)
  • bg2 journal: saved notes do not appear in the journal
  • bg2 journal: you can increase chapter number arbitrarily in a new tob game (unset Chapter?)
  • tob start: when you talk to the heads, the eye flames should not loop the onset animation
  • iwd2 if you're not in the primary spellbook, 1st level (eg. in the domain spells tab or at higher spell level), clicking anywhere will instead switch to it
  • stores lost their minimum price of 1. Eg. arrow stacks in iwd2 ar1102 cost 0.
  • iwd2 hp overlay colors are inverted:
    image
    image
  • bg1/bg2 party reform usually starts with buttons disabled (they enable after some clicking around) image
  • pst party reform doesn't work any more (also (too) small portraits aren't drawn on the buttons)
clicking on the scrollbar in feat selection crashes in python

#0  0x00007fffef9d11a8 in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#1  0x00007fffef97b3cd in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#2  0x00007fffef97b62a in _PyObject_FastCallDictTstate () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#3  0x00007fffef97b810 in _PyObject_Call_Prepend () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#4  0x00007fffef9ee3ca in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#5  0x00007fffef9de432 in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#6  0x00007fffef97b888 in _PyObject_Call () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#7  0x00007ffff431ae84 in GemRB::GUIScript::ConstructObject (this=0x555555858030, pyclassname="Window", 
    pArgs=0x7fffeff46c78 <_PyRuntime+58904>, kwArgs=0x7fffef503680)
    at gemrb/plugins/GUIScript/GUIScript.cpp:13780
#8  0x00007ffff431abfa in GemRB::GUIScript::ConstructObject (this=0x555555858030, pyclassname="Window", 
    id=55) at gemrb/plugins/GUIScript/GUIScript.cpp:13758
#9  0x00007ffff431a932 in GemRB::GUIScript::ConstructObjectForScriptable (this=0x555555858030, 
    ref=0x5555589f5de0) at gemrb/plugins/GUIScript/GUIScript.cpp:13742
#10 0x00007ffff431a547 in GemRB::GUIScript::AssignViewAttributes (this=0x555555858030, obj=0x7fffef5131b0, 
    view=0x555558a029e0) at gemrb/plugins/GUIScript/GUIScript.cpp:13715
#11 0x00007ffff431aaa5 in GemRB::GUIScript::ConstructObjectForScriptable (this=0x555555858030, 
    ref=0x555558a02da0) at gemrb/plugins/GUIScript/GUIScript.cpp:13749
--Type <RET> for more, q to quit, c to continue without paging--
#12 0x00007ffff42e85fc in GemRB_GetView (args=0x7fffef6de0c0)
    at gemrb/plugins/GUIScript/GUIScript.cpp:1188
#13 0x00007fffef9c70c5 in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#14 0x00007fffef9786c1 in _PyObject_MakeTpCall () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#15 0x00007fffef90bbb7 in _PyEval_EvalFrameDefault () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#16 0x00007fffefa68d82 in ?? () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#17 0x00007fffef97bbaf in PyObject_CallObject () from /lib/x86_64-linux-gnu/libpython3.11.so.1.0
#18 0x00007ffff43263ee in GemRB::CallPython<int, &GemRB::noop<int> > (function=0x7fffef554ae0, args=0x0, 
    retVal=0x7fffffffb8c4) at gemrb/plugins/GUIScript/PythonCallbacks.h:45
#19 0x00007ffff43204c3 in GemRB::CallPython (function=0x7fffef554ae0, args=0x0)
    at gemrb/plugins/GUIScript/PythonCallbacks.h:65
#20 0x00007ffff43672ea in GemRB::PythonComplexCallback<void, GemRB::Control*>::operator() (
    this=0x555558992bf0, arg=0x5555589e19a0)
    at gemrb/plugins/GUIScript/PythonCallbacks.h:127
#21 0x00007ffff435d2f2 in std::__invoke_impl<void, GemRB::PythonComplexCallback<void, GemRB::Control*>&, GemRB::Control*> (__f=...) at /usr/include/c++/12/bits/invoke.h:61
#22 0x00007ffff43520b7 in std::__invoke_r<void, GemRB::PythonComplexCallback<void, GemRB::Control*>&, GemRB::Control*> (__fn=...) at /usr/include/c++/12/bits/invoke.h:154
--Type <RET> for more, q to quit, c to continue without paging--
#23 0x00007ffff4346d79 in std::_Function_handler<void (GemRB::Control*), GemRB::PythonComplexCallback<void, GemRB::Control*> >::_M_invoke(std::_Any_data const&, GemRB::Control*&&) (__functor=..., 
    __args#0=@0x7fffffffb990: 0x5555589e19a0) at /usr/include/c++/12/bits/std_function.h:290
#24 0x00007ffff79a0949 in std::function<void (GemRB::Control*)>::operator()(GemRB::Control*) const (
    this=0x555558992ba8, __args#0=0x5555589e19a0) at /usr/include/c++/12/bits/std_function.h:591
#25 0x00007ffff799fb4f in GemRB::View::ActionResponder<GemRB::Control*>::Responder::operator() (
    this=0x555558992ba8, responder=0x5555589e19a0)
    at gemrb/core/GUI/ViewInterfaces.h:74
#26 0x00007ffff799cc50 in GemRB::Control::PerformAction (this=0x5555589e19a0, key=...)
    at gemrb/core/GUI/Control.cpp:101
#27 0x00007ffff799ce85 in GemRB::Control::SetValue (this=0x5555589e19a0, val=37)
    at gemrb/core/GUI/Control.cpp:141
#28 0x00007ffff79cfc2a in GemRB::ScrollBar::ScrollTo (this=0x5555589e19a0, p=...)
    at gemrb/core/GUI/ScrollBar.cpp:88
#29 0x00007ffff79d0826 in GemRB::ScrollBar::OnMouseDrag (this=0x5555589e19a0, me=...)
    at gemrb/core/GUI/ScrollBar.cpp:256
#30 0x00007ffff7a01068 in GemRB::View::MouseDrag (this=0x5555589e19a0, me=...)
    at gemrb/core/GUI/View.cpp:739
--Type <RET> for more, q to quit, c to continue without paging--
#31 0x00007ffff7a097a0 in GemRB::Window::DispatchMouseMotion (this=0x55555801fef0, target=0x0, me=...)
    at gemrb/core/GUI/Window.cpp:363
#32 0x00007ffff7a089ac in GemRB::Window::DispatchEvent (this=0x55555801fef0, event=...)
    at gemrb/core/GUI/Window.cpp:535
#33 0x00007ffff7a10eb6 in GemRB::WindowManager::DispatchEvent (this=0x555555b228c0, event=...)
    at gemrb/core/GUI/WindowManager.cpp:420
#34 0x00007ffff7a1b831 in std::__invoke_impl<bool, bool (GemRB::WindowManager::*&)(GemRB::Event const&), GemRB::WindowManager*&, GemRB::Event const&> (
    __f=@0x555558a09e30: (bool (GemRB::WindowManager::*)(GemRB::WindowManager * const, const GemRB::Event &)) 0x7ffff7a10cd8 <GemRB::WindowManager::DispatchEvent(GemRB::Event const&)>, 
    __t=@0x555558a09e40: 0x555555b228c0) at /usr/include/c++/12/bits/invoke.h:74
#35 0x00007ffff7a1b159 in std::__invoke<bool (GemRB::WindowManager::*&)(GemRB::Event const&), GemRB::WindowManager*&, GemRB::Event const&> (
    __fn=@0x555558a09e30: (bool (GemRB::WindowManager::*)(GemRB::WindowManager * const, const GemRB::Event &)) 0x7ffff7a10cd8 <GemRB::WindowManager::DispatchEvent(GemRB::Event const&)>)
    at /usr/include/c++/12/bits/invoke.h:96
#36 0x00007ffff7a1a632 in std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>::__call<bool, GemRB::Event const&, 0ul, 1ul>(std::tuple<GemRB::Event const&>&&, std--Type <RET> for more, q to quit, c to continue without paging--
::_Index_tuple<0ul, 1ul>) (this=0x555558a09e30, __args=...) at /usr/include/c++/12/functional:495
#37 0x00007ffff7a19970 in std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>::operator()<GemRB::Event const&, bool>(GemRB::Event const&) (this=0x555558a09e30)
    at /usr/include/c++/12/functional:580
#38 0x00007ffff7a1846b in std::__invoke_impl<bool, std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>&, GemRB::Event const&>(std::__invoke_other, std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>&, GemRB::Event const&) (__f=...) at /usr/include/c++/12/bits/invoke.h:61
#39 0x00007ffff7a166cd in std::__invoke_r<bool, std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>&, GemRB::Event const&>(std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)>&, GemRB::Event const&) (__fn=...)
    at /usr/include/c++/12/bits/invoke.h:142
#40 0x00007ffff7a14e31 in std::_Function_handler<bool (GemRB::Event const&), std::_Bind<bool (GemRB::WindowManager::*(GemRB::WindowManager*, std::_Placeholder<1>))(GemRB::Event const&)> >::_M_invoke(std::_Any_data const&, GemRB::Event const&) (__functor=..., __args#0=...) at /usr/include/c++/12/bits/std_function.h:290
#41 0x00007ffff79a5f8d in std::function<bool (GemRB::Event const&)>::operator()(GemRB::Event const&) const (
    this=0x5555589ec580, __args#0=...) at /usr/include/c++/12/bits/std_function.h:591
#42 0x00007ffff79a44f8 in GemRB::EventMgr::DispatchEvent (this=0x555555b229c0, e=...)
--Type <RET> for more, q to quit, c to continue without paging--
    at gemrb/core/GUI/EventMgr.cpp:272
#43 0x00007ffff5995d19 in GemRB::SDLVideoDriver::ProcessEvent (this=0x5555555a9400, event=...)
    at gemrb/plugins/SDLVideo/SDLVideo.cpp:193
#44 0x00007ffff59c941d in GemRB::SDL20VideoDriver::ProcessEvent (this=0x5555555a9400, event=...)
    at gemrb/plugins/SDLVideo/SDL20Video.cpp:923
#45 0x00007ffff59957c8 in GemRB::SDLVideoDriver::PollEvents (this=0x5555555a9400)
    at gemrb/plugins/SDLVideo/SDLVideo.cpp:58
#46 0x00007ffff7c4d672 in GemRB::Video::SwapBuffers (this=0x5555555a9400, fpscap=0)
    at gemrb/core/Video/Video.cpp:159
#47 0x00007ffff7ad36bb in GemRB::Interface::Main (this=0x7fffffffcc90)
    at gemrb/core/Interface.cpp:1013
#48 0x000055555555bacb in main (argc=3, argv=0x7fffffffd968) at gemrb/GemRB.cpp:66 

starting an iwd2 game breaks the gui

[Python/ERROR]: Traceback (most recent call last):
[Python/ERROR]:   File "/home/kajak/games/gemrb/build/../gemrb/GUIScripts/iwd2/MessageWindow.py", line 97, in UpdateControlStatus
[Python/ERROR]:     largeMW.SetVisible(False)
[Python/ERROR]:     ^^^^^^^^^^^^^^^^^^
[Python/ERROR]: AttributeError: 'NoneType' object has no attribute 'SetVisible'

you can't drink in taverns

[Python/ERROR]: Traceback (most recent call last):
[Python/ERROR]:   File "/home/kajak/games/gemrb/build/../gemrb/GUIScripts/GUISTORE.py", line 1890, in GulpDrink
[Python/ERROR]:     Index = btn.Value + GemRB.GetVar("TopIndex")
[Python/ERROR]:             ^^^
[Python/ERROR]: NameError: name 'btn' is not defined. Did you mean: 'bin'?

some vars didn't get moved together

[Python/ERROR]: Traceback (most recent call last):
[Python/ERROR]:   File "/home/kajak/games/gemrb/build/../gemrb/GUIScripts/PortraitWindow.py", line 388, in UpdatePortraitWindow
[Python/ERROR]:     for Button in GetPortraitButtons(Window):
[Python/ERROR]:                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
[Python/ERROR]:   File "/home/kajak/games/gemrb/build/../gemrb/GUIScripts/PortraitWindow.py", line 143, in GetPortraitButtons
[Python/ERROR]:     if windowHeight != ScreenHeight:
[Python/ERROR]:                        ^^^^^^^^^^^^
[Python/ERROR]: NameError: name 'ScreenHeight' is not defined

@@ -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
Copy link
Member

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
Copy link
Member

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.

Comment on lines +104 to +105
ScriptID spkID = SetSpeaker(spk);
targetID = SetTarget(tgt);
Copy link
Member

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:

Suggested change
ScriptID spkID = SetSpeaker(spk);
targetID = SetTarget(tgt);
SetSpeaker(spk);
SetTarget(tgt);

(with the other spkID change reverted)

@@ -404,8 +403,10 @@ bool DialogHandler::DialogChoose(unsigned int choose)
EndDialog();
return false;
}

auto targetID = core->GetDictionary().GetAs("DLG_TARGET_ID", 0);
Copy link
Member

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
Copy link
Member

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
Copy link

sonarcloud bot commented Oct 29, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug D 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 54 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@lynxlynxlynx
Copy link
Member

Brad, can you finish this soonish? It keeps getting conflicts against master, since it's a large body of work

@bradallred
Copy link
Member Author

I'll try to find some time to dust it off this weekend.

Copy link

sonarcloud bot commented Mar 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@lynxlynxlynx
Copy link
Member

Do you need help, should we just postpone, something else?

Copy link

sonarcloud bot commented May 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

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.

Animated portraits drawing regression GUIScript: add a "Window" field to GView
3 participants