OpenGothic icon indicating copy to clipboard operation
OpenGothic copied to clipboard

Published 20 hours ago • verified publisher icon Try

Migrate from ZenLib to lmichaelis/phoenix

Open lmichaelis opened this issue 3 years ago • 34 comments

This is a PR for #269. All parsers and classes exposed by ZenLib. Currently, the following known bugs are being worked on.

Still to do

  • [x] ~~Test build on MSVC/Windows~~
  • [x] ~~Migrate from ZenLib's DATFile and DaedalusVM to phoenix's script and daedalus_interpreter~~
    • [x] ~~CAMERA.DAT~~
    • [x] ~~FIGHT.DAT~~
    • [x] ~~GOTHIC.DAT~~
    • [x] ~~MENU.DAT~~
    • [x] ~~MUSIC.DAT~~
    • [x] ~~PARTICLEFX.DAT~~
    • [x] ~~SFX.DAT~~
    • [x] ~~VISUALFX.DAT~~
  • [ ] Properly test Gothic 1
    • [x] ~~phoenix fails to load multiple MDS files because of missing closing parentheses~~ **Fixed in [phoenix/bugfix/fix-missing-parens-#9](https://github.com/lmichaelis/phoenix/tree/bugfix/fix-missing-parens-%
  • [x] ~~Test savegame compatiblity~~
    • [x] ~~OpenGothic hangs on load~~
    • [x] ~~OpenGothic fails to load saves made before phoenix was added. This is due to differing Vob type IDs in Vob::load.~~
    • [x] ~~The way script symbols are written to the save file has changed. The new system is not able to read DataContainer<T> values properly because they don't exist any more. This might also apply to instance type symbols.~~
    • [x] ~~Containers specifically disappear when loading an old save.~~
  • [x] ~~Line breaks aren't rendered properly in the quest log window~~
  • [x] ~~Trying to read the stone tablet from Xardas' tower causes a crash - probably due to missing fonts~~

Things which will be addressed in other PRs

  • Investigate poor performance in Debug builds => Sanitizers?
  • Optimize Vob-tree by reducing copies
  • General optimization
  • Investigate missing SFX for smithing and possibly other animations (some, like sawing, work however)
  • Rework dialog sub-choices For compatibility, these have been implemented in phoenix but they don't belong there. An OpenGothic class should be reused for them (DlgChoise)?
Old Bugs
  • [x] [fixed] ~~Targeting does not work correctly~~

image (the bed should be targeted)

  • [x] [fixed] ~~Animations can cause characters to clip into objects~~

https://user-images.githubusercontent.com/20187490/172038524-e23750f8-d87c-4e65-80c4-bee0e8d9ad5c.mp4

  • [x] [fixed] ~~Some meshes are stretchy (this must have something to do with their positioning/rotation in the world)~~

image

  • [x] [fixed] ~~Doors don't show up~~

image

  • [x] [fixed] ~~Some lights have an invalid range setting~~

image

  • [x] [fixed] ~~Some NPCs seem to sink below ground~~
  • [x] [fixed] ~~Some models seem to be missing :(~~

image

  • [x] [fixed] ~~Weapon draw animation / fight mode does not work~~

https://user-images.githubusercontent.com/20187490/172038766-8c4841f0-cf6c-44f3-8c9b-59e160f004ef.mp4

  • [x] ~~Saves fail to load~~
  • [x] [fixed] ~~A lof of NPCs end up at the harbor (missing waypoints?)~~
  • [x] [fixed] ~~The game selects the wrong menu background~~

I've found the issue. It happens due to Tempest being unable to handle DDS textures containing any other format than DXT1, DXT3 or DXT5. If Tempest can't handle the texture, OpenGothic will instead try to load the TGA-version of that texture which is present twice in the VDFs:

image

Just by coincidence, PhysicsFS returns the addon's start screen texture instead of the base-game's. This behavior can not be implemented in phoenix, I believe.

lmichaelis avatar Jun 04 '22 15:06 lmichaelis

Can't spot much from diff - for the most part looks very much like 1-to-1 replacements. Possible bugs inside the library itself.

Targeting does not work correctly

You mean focus, like pick-item/bow/attack? Maybe, WorldObjects::SearchOpt::collectAlgo is invalid, but this part is script driven...

Animations can cause characters to clip into objects Some NPCs seem to sink below ground

Bbox?

Doors don't show up Weapon draw animation / fight mode does not work

Feels, like animation/hierarchy related stuff gone wrong. Fight mode is controlled by animation events(ZenLoad::EFightMode), see Animation::Sequence::processEvent.

For door: is it only door or other objects are also problematic? What about rune table?

PS I'm skipping local debugging for now, since there are merge conflicts.

Try avatar Jun 04 '22 22:06 Try

Ah, sorry, I forgot to include more information. I've updated with screenshots and recording to explain the problem. I've also added the commit I first noticed this to make troubleshooting easier :)

Thanks for your suggestions :) I'll look into it.

lmichaelis avatar Jun 05 '22 06:06 lmichaelis

Okay @Try, I've managed to fix most of the bugs :tada: I can't seem to find the cause of the broken fight animations though. Could you take a look at them?

lmichaelis avatar Jun 05 '22 20:06 lmichaelis

Hi there, just checking in since I have been working very slowly over the past few months. I am still working on this and I just had a breakthrough migrating the script VM which I will be publishing soon.

I will open an issue tomorrow, however since I have run into another problem related to scripting which also exists on the main branch.

I'm hoping I can get this PR ready this or next month so we can proceed to testing it :)

lmichaelis avatar Aug 14 '22 19:08 lmichaelis

Hi, @lmichaelis !

Nice to hear news from you :)

I'm hoping I can get this PR ready this or next month so we can proceed to testing it :)

Take your time - meanwhile I'm still busy with graphics/optimization stuff - shouldn't interfere with scripting too much

Try avatar Aug 14 '22 22:08 Try

I've got another update on my progress! I've been able to full eliminate ZenLib from the project without too many issues. I've now updated phoenix, so it properly builds on MSVC / Visual Studio. I've also been able to fix the fight mode glitch that's been bugging me for months. Turns out, I was setting EvCount::weaponCh to event_fight_mode::none by default which caused the player to put back his weapon as soon as the animation finished.

I'm currently working on stabilizing phoenix's API as well as implementing a proper error handling system for the VM. I've also encountered a rather severe problem with Gothic I MDS files which I need to fix. I'm keeping track of known issues up in the initial pull request, if you're curious :)

lmichaelis avatar Sep 08 '22 17:09 lmichaelis

Heyo @Try, I have great news! OpenGothic now fully runs Gothic II using phoenix! I've been able to work a lot of problems and I'm happy to say that all issues I was aware of in Gothic II have now been fixed. While testing my changes I've come across some other issues that are also present on master and I'll probably work on them too once this is all done :slightly_smiling_face:

I am currently aware of bugs with Gothic I, however. With a patch available at phoenix/bugfix/fix-missing-parens-#9, Gothic I loads at least but it seems to be kind of unstable. Comparing it to master, most of these issues seem to exist there too though, so here's a question: should I invest time into trying to figure these issues out right now or should I do that later?

What I have done in addition to just replacing old with new:

  • Bump Serialize::Version::Current to 39
  • Ignore consistency checks for VOb types in Vob::load(Serialize&) for save versions <39.
  • Ignored saved visual for oCMobContainer VObs for save versions <39 (if it is included, containers go missing)
  • Added game/compatibility/phoenix.{h,cpp} to house functions needed for phoenix compatibility
  • Removed functions and constants no longer needed

The only things left for you to do on this PR then are these:

  • Fix the CI pipeline (it's not set up to support C++20 yet)
  • Review/Test my changes (that's gonna be a lot of work :laughing:), most importantly that it still compiles on your end, that the world loads up and that you can load a save made with a previous version of OpenGothic
  • Merge this PR :wink:

lmichaelis avatar Sep 10 '22 16:09 lmichaelis

Hi, @lmichaelis !

OpenGothic now fully runs Gothic II using phoenix!

Those are very good news! In terms of testing I have a suggestion: since it's been a while since last stable build, good approach can be to publish incremental update in September without phenix, and only then make a merge to master. That way, we ca have ~3 months for testing, until winter update.

Try avatar Sep 10 '22 18:09 Try

That sounds good. I'll play as much of the game as I can looking for issues. Ideally you would play a bit too, since I've already written down a lot of things I would classify as bugs but they appear in both this patched version as well as on master. It's gonna be hard for me to distinguish what's caused by phoenix and what's already there.

And about Gothic I support: Do you want to support it right now or is the focus more on Gothic II?

lmichaelis avatar Sep 10 '22 18:09 lmichaelis

And about Gothic I support: Do you want to support it right now or is the focus more on Gothic II?

If we gonna to merge right after next update, than we can ignore G1 at merge time, and fix it on a go

Try avatar Sep 11 '22 18:09 Try

Hi, I'm currently playing through G2 on OpenGothic, so I can help testing. However, I am using L'Hiver Edition 0.9 mod, which works fine with current master, but phoenix get stuck during loading or starting a new game in an infinite loop in archive_reader::skip_object (archive.cc)

[phoenix] world: object [% oCWorld:zCWorld 36865 0] not fully parsed

For comparison the log output with zenlib:

Info: Reading 21032 blocks
Info: ZEN: Reading world...
Info: oCWorld reading chunk: MeshAndBsp
Info: ZEN: Reading mesh...
Info: Reading mesh '' (Version: 265)
Info: Found 49751 vertices
Info: ZEN: Done reading mesh!
Info: oCWorld reading chunk: VobTree
Info: oCWorld reading chunk: WayNet
Info: Loading 16 freepoints
Info: Loading 1819 edges
Info: Done loading edges!
Info: ZEN: Reading presets...
unable to load Zen-file: "FIRETREE_LARGE.ZEN"
Info: ZEN: Reading world...
Info: oCWorld reading chunk: VobTree
Info: oCWorld reading chunk: EndMarker

matthiakl avatar Sep 11 '22 19:09 matthiakl

Hi @matthiakl, thanks for the report! I've been able to reproduce the problem and I've traced it to an improper implementation of ASCII-formatted ZenGin Archives. That problem has now been fixed. Additionally, I've fixed a memory corruption issue that happened with non-DXT textures right after loading in as well as an issue which would cause objects to be rotated in nonsensical ways.

You're of course welcome to help, but please keep in mind that any saves you make with this new version will not be backwards compatible. I would recommend making a backup of your current save files before testing :)

lmichaelis avatar Sep 12 '22 16:09 lmichaelis

but please keep in mind that any saves you make with this new version will not be backwards compatible

Thanks for the advice. And the loading now works. I also have the impression, that load and save times are faster.

Here is the next bug ;) When I try to cast a spell with a scroll, I get a segfault. This is also reproducible in a new vanilla game with the spell in the library of Xardas' tower.

Thread 1 (Thread 0x7ffff5fbe400 (LWP 77761) "Gothic2Notr"):
#0  0x0000555556ca6fe8 in std::__shared_ptr<phoenix::daedalus::c_npc, (__gnu_cxx::_Lock_policy)2>::__shared_ptr (this=0x7fffffff8bb0) at /usr/include/c++/12.2.0/bits/shared_ptr_base.h:1522
No locals.
#1  0x0000555556ca7033 in std::shared_ptr<phoenix::daedalus::c_npc>::shared_ptr (this=0x7fffffff8bb0) at /usr/include/c++/12.2.0/bits/shared_ptr.h:204
No locals.
#2  0x0000555556c97d42 in GameScript::invokeMana (this=0x6230000efd00, npc=..., target=0x0) at /mnt/storage/Programme/OpenGothic/game/game/gamescript.cpp:974
        fn = 0x7fffe456bc80
        self = {prev = {<std::__shared_ptr<phoenix::daedalus::instance, (__gnu_cxx::_Lock_policy)2>> = {<std::__shared_ptr_access<phoenix::daedalus::instance, (__gnu_cxx::_Lock_policy)2, false, false>> = {<No data fields>}, _M_ptr = 0x0, _M_refcount = {_M_pi = 0x0}}, <No data fields>}, sym = 0x7fffe4473580}
        other = {prev = {<std::__shared_ptr<phoenix::daedalus::instance, (__gnu_cxx::_Lock_policy)2>> = {<std::__shared_ptr_access<phoenix::daedalus::instance, (__gnu_cxx::_Lock_policy)2, false, false>> = {<No data fields>}, _M_ptr = 0x619000483a90, _M_refcount = {_M_pi = 0x619000483a80}}, <No data fields>}, sym = 0x7fffffff8c24}
#3  0x0000555556efe200 in Npc::beginCastSpell (this=0x61f000230e80) at /mnt/storage/Programme/OpenGothic/game/world/objects/npc.cpp:3209
        active = 0x61200016c0c0
        code = 32767
#4  0x0000555556d5c4e0 in PlayerControl::implMove (this=0x7fffffffe010, dt=49) at /mnt/storage/Programme/OpenGothic/game/game/playercontrol.cpp:661
#5  0x0000555556d5b943 in PlayerControl::tickMove (this=0x7fffffffe010, dt=49) at /mnt/storage/Programme/OpenGothic/game/game/playercontrol.cpp:472
#6  0x0000555556e5a503 in MainWindow::tick (this=0x7fffffffaf30) at /mnt/storage/Programme/OpenGothic/game/mainwindow.cpp:763
#7  0x0000555556e5b994 in MainWindow::render (this=0x7fffffffaf30) at /mnt/storage/Programme/OpenGothic/game/mainwindow.cpp:988
#8  0x00007ffff73857ff in Tempest::EventDispatcher::dispatchRender (this=0x7ffff78eb380 <dispatcher>, wnd=...) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/eventdispatcher.cpp:202
#9  0x00007ffff738afb4 in Tempest::SystemApi::dispatchRender (w=...) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/systemapi.cpp:110
#10 0x00007ffff737ff5e in Tempest::X11Api::implProcessEvents (this=0x7ffff78eb420 <Tempest::SystemApi::inst()::api>, cb=...) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/api/x11api.cpp:511
#11 0x00007ffff737f70a in Tempest::X11Api::implExec (this=0x7ffff78eb420 <Tempest::SystemApi::inst()::api>, cb=...) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/api/x11api.cpp:383
#12 0x00007ffff738b239 in Tempest::SystemApi::exec (cb=...) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/systemapi.cpp:167
#13 0x00007ffff738274d in Tempest::Application::exec (this=0x7fffffff9248) at /mnt/storage/Programme/OpenGothic/lib/Tempest/Engine/system/application.cpp:75
#14 0x0000555556e53856 in main (argc=6, argv=0x7fffffffe3b8) at /mnt/storage/Programme/OpenGothic/game/main.cpp:109

matthiakl avatar Sep 12 '22 20:09 matthiakl

Thanks @matthiakl! There were some missing null-checks when spells don't have a target.

lmichaelis avatar Sep 13 '22 14:09 lmichaelis

Some more issues:

  • If I start the default GothicGame.ini for NotR, the titlescreen shows the vanilla image instead of the one from NotR, but the addon is correctly loaded when a new game is started.
  • Some weapons on NPCs are not aligned correctly and point into the air. After talking to them, the weapons are updated and aligned correctly to the back. This might be a savegame conversion issue (see first picture).
  • Indoor lightning from small light sources seems to be missing as you can see in the comparison of the two images below. I only observed this issue in my L'Hiver edition, but not in vanilla. Screenshot with phoenix: Bildschirmfoto vom 2022-09-13 18-22-04 Screenshot with ZenLib: Bildschirmfoto vom 2022-09-13 18-29-35

matthiakl avatar Sep 13 '22 17:09 matthiakl

Yeah that first one is nasty. It has something to do with the load order of VDF files. I had fixed it at some point but it seems it's come back. Can you send over a copy of your save game so I can investigate? Thanks!

lmichaelis avatar Sep 13 '22 18:09 lmichaelis

Here is my savegame from the ZenLib version at the fortress. There you can experience both issues. Also I was finally able to locate the original download page for my mods, with them you should be able to load it. save_slot_4.sav.zip (just renamed, otherwise Github refuses it)

L'Hiver Edition DE 0.91 - Deutsche Version

https://forum.worldofplayers.de/forum/threads/1496232-L-Hiver-Edition-DE-0-91-Deutsche-Version-3

Downloads from that page:

  • L'Hiver Edition DE 0.91: Mit grünem Minental http://www.mediafire.com/file/1vxn7gz6kodqf53/LHiver_Edition_DE_Original_Green.7z
  • BalancingFix 2: http://www.mediafire.com/file/tveta76z51zxijb/LHiver_BalancingFix_2.7z

matthiakl avatar Sep 14 '22 05:09 matthiakl

I am unable to load Jharkendar:

Log output 
[phoenix] world: parsing object [VobTree % 0 0]
[phoenix] vob_tree: encountered unknown VOb [% � 0 9824]
[phoenix] vob_tree: encountered unknown VOb [% � 0 9823]
[phoenix] world: parsing object [WayNet % 0 0]
[phoenix] world: parsing object [EndMarker % 0 0]
unable to load landscape mesh
loading error: out of memory
=================================================================
==16080==ERROR: AddressSanitizer: requested allocation size 0x4000000000010001 (0x4000000000011008 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T21)
    #0 0x7ffff79ac672 in operator new(unsigned long) /usr/src/debug/gcc/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x555556bf48a4 in std::__new_allocator::allocate(unsigned long, void const*) /usr/include/c++/12.2.0/bits/new_allocator.h:137
    #2 0x555556bf35d8 in std::allocator::allocate(unsigned long) /usr/include/c++/12.2.0/bits/allocator.h:188
    #3 0x555556bf35d8 in std::allocator_traits<:allocator> >::allocate(std::allocator&, unsigned long) /usr/include/c++/12.2.0/bits/alloc_traits.h:464
    #4 0x555556bf3247 in std::__cxx11::basic_string, std::allocator >::_M_create(unsigned long&, unsigned long) /usr/include/c++/12.2.0/bits/basic_string.tcc:155
    #5 0x555556bf3683 in std::__cxx11::basic_string, std::allocator >::_M_assign(std::__cxx11::basic_string, std::allocator > const&) /usr/include/c++/12.2.0/bits/basic_string.tcc:284
    #6 0x555556bf28b2 in std::__cxx11::basic_string, std::allocator >::assign(std::__cxx11::basic_string, std::allocator > const&) /usr/include/c++/12.2.0/bits/basic_string.h:1571
    #7 0x555556c431e2 in std::__cxx11::basic_string, std::allocator >::operator=(std::__cxx11::basic_string, std::allocator > const&) /usr/include/c++/12.2.0/bits/basic_string.h:805
    #8 0x555556f16235 in AbstractTrigger::AbstractTrigger(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/abstracttrigger.cpp:34
    #9 0x555556f1f30c in TouchDamage::TouchDamage(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/touchdamage.cpp:8
    #10 0x555556f1074f in Vob::load(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:171
    #11 0x555556f0fa7a in Vob::Vob(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:49
    #12 0x555556f15bd1 in AbstractTrigger::AbstractTrigger(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/abstracttrigger.cpp:14
    #13 0x555556f1bd23 in MoveTrigger::MoveTrigger(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/movetrigger.cpp:12
    #14 0x555556f1047f in Vob::load(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:153
    #15 0x555556f0fa7a in Vob::Vob(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:49
    #16 0x555556f15bd1 in AbstractTrigger::AbstractTrigger(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/abstracttrigger.cpp:14
    #17 0x555556f1f832 in Trigger::Trigger(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/triggers/trigger.cpp:10
    #18 0x555556f1065f in Vob::load(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:165
    #19 0x555556f0fa7a in Vob::Vob(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:49
    #20 0x555556f1033f in Vob::load(Vob*, World&, std::unique_ptr<:vob std::default_delete> > const&, Vob::Flags) /mnt/storage/Programme/OpenGothic/game/world/objects/vob.cpp:136
    #21 0x555556f3f414 in WorldObjects::addRoot(std::unique_ptr<:vob std::default_delete> > const&, bool) /mnt/storage/Programme/OpenGothic/game/world/worldobjects.cpp:646
    #22 0x555556f31432 in World::World(GameSession&, std::__cxx11::basic_string, std::allocator >, bool, std::function) /mnt/storage/Programme/OpenGothic/game/world/world.cpp:94
    #23 0x555556d38fc3 in GameSession::implChangeWorld(std::unique_ptr >&&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&) /mnt/storage/Programme/OpenGothic/game/game/gamesession.cpp:336
    #24 0x555556d38728 in operator() /mnt/storage/Programme/OpenGothic/game/game/gamesession.cpp:303
    #25 0x555556d3ac22 in __invoke_impl<:unique_ptr>, GameSession::tick(uint64_t)::&&)>&, std::unique_ptr > > /usr/include/c++/12.2.0/bits/invoke.h:61
    #26 0x555556d3a7d9 in __invoke_r<:unique_ptr>, GameSession::tick(uint64_t)::&&)>&, std::unique_ptr > > /usr/include/c++/12.2.0/bits/invoke.h:116
    #27 0x555556d3a2b4 in _M_invoke /usr/include/c++/12.2.0/bits/std_function.h:291
    #28 0x555556d7bb73 in std::function<:unique_ptr std::default_delete> > (std::unique_ptr >&&)>::operator()(std::unique_ptr >&&) const (/mnt/storage/Programme/OpenGothic/build-debug/opengothic/Gothic2Notr+0x1827b73)
    #29 0x555556d6ab53 in operator() /mnt/storage/Programme/OpenGothic/game/gothic.cpp:417
    #30 0x555556d792d1 in __invoke_impl(std::unique_ptr&&)>):: > /usr/include/c++/12.2.0/bits/invoke.h:61

Some Vobs seem to have no target member and cannot be cast to trigger properly. By simply ignoring the error, the world loads, but I am not sure, which side effects may remain.

Debug Info @OpenGothic/game/world/triggers/abstracttrigger.cpp:34 
this	@0x7ffeb8f64700	AbstractTrigger
	[Vob]	@0x7ffeb8f64700	Vob
	bboxOrigin	@0x7ffeb8f64864	Tempest::Vec3
	bboxSize	@0x7ffeb8f64858	Tempest::Vec3
	box	@0x7ffeb8f647c8	DynamicWorld::BBoxBody
	boxNpc	@0x7ffeb8f647e8	CollisionZone
	callback	@0x7ffeb8f647b8	AbstractTrigger::Cb
	disabled	false	bool
	emitCount	0	uint32_t
	emitTimeLast	0	uint64_t
	filterFlags	0	uint32_t
	fireDelaySec	0	float
	maxActivationCount	825294901	uint32_t
	target	""	std::string
	triggerFlags	53	uint32_t
	vobName	""	std::string
trigger	@0x7ffec68d15e0	phoenix::vobs::touch_damage
	[phoenix::vob]	@0x7ffec68d15e0	phoenix::vob
		[vptr]	_vptr.vob	 
			[0]	0x55555620d69e <:vobs::touch_damage::>	void*
			[1]	0x55555620d6cc <:vobs::touch_damage::>	void*
		ambient	false	bool
		anim_mode	phoenix::animation_mode::none (0)	phoenix::animation_mode
		anim_strength	0	float
		associated_visual_type	phoenix::visual_type::unknown (7)	phoenix::visual_type
		bbox	@0x7ffec68d15f0	phoenix::bounding_box
		bias	0	int32_t
		camera_alignment	phoenix::camera_lock_mode::none (0)	phoenix::camera_lock_mode
		cd_dynamic	true	bool
		cd_static	false	bool
		children		std::vector<:unique_ptr std::default_delete> >, std::allocator<:unique_ptr std::default_delete> > > >
		dynamic_shadows	phoenix::shadow_mode::none (0)	phoenix::shadow_mode
		far_clip_scale	1	float
		id	11108	uint32_t
		physics_enabled	false	bool
		position	@0x7ffec68d1608	glm::vec3
		preset_name	""	std::string
		rotation	@0x7ffec68d1614	glm::mat3x3
		show_visual	true	bool
		type	phoenix::vob_type::oCTouchDamage (17)	phoenix::vob_type
		visual_decal		std::optional<:decal>
		visual_name	""	std::string
		vob_name	""	std::string
		vob_static	false	bool
	barrier	false	bool
	blunt	false	bool
	collision	phoenix::collision_type::box (1)	phoenix::collision_type
	damage	1000	float
	edge	false	bool
	fall	false	bool
	fire	false	bool
	fly	false	bool
	magic	false	bool
	point	true	bool
	repeat_delay_sec	2	float
	volume_scale	1	float
trigger@1	@0x7ffec68d15e0	phoenix::vobs::touch_damage
	[phoenix::vob]	@0x7ffec68d15e0	phoenix::vob
		[vptr]	_vptr.vob	 
		ambient	false	bool
		anim_mode	phoenix::animation_mode::none (0)	phoenix::animation_mode
		anim_strength	0	float
		associated_visual_type	phoenix::visual_type::unknown (7)	phoenix::visual_type
		bbox	@0x7ffec68d15f0	phoenix::bounding_box
		bias	0	int32_t
		camera_alignment	phoenix::camera_lock_mode::none (0)	phoenix::camera_lock_mode
		cd_dynamic	true	bool
		cd_static	false	bool
		children		std::vector<:unique_ptr std::default_delete> >, std::allocator<:unique_ptr std::default_delete> > > >
		dynamic_shadows	phoenix::shadow_mode::none (0)	phoenix::shadow_mode
		far_clip_scale	1	float
		id	11108	uint32_t
		physics_enabled	false	bool
		position	@0x7ffec68d1608	glm::vec3
		preset_name	""	std::string
		rotation	@0x7ffec68d1614	glm::mat3x3
		show_visual	true	bool
		type	phoenix::vob_type::oCTouchDamage (17)	phoenix::vob_type
		visual_decal		std::optional<:decal>
		visual_name	""	std::string
		vob_name	""	std::string
		vob_static	false	bool
	barrier	false	bool
	blunt	false	bool
	collision	phoenix::collision_type::box (1)	phoenix::collision_type
	damage	1000	float
	edge	false	bool
	fall	false	bool
	fire	false	bool
	fly	false	bool
	magic	false	bool
	point	true	bool
	repeat_delay_sec	2	float
	volume_scale	1	float

matthiakl avatar Sep 14 '22 20:09 matthiakl

Hio, Jharkendar is now fixed alongside some other potential bugs. The background image on the main menu is now displayed correctly as well. ~~I have run into a weird issue with it, however as I'll explain below.~~

I can confirm that the weird sword orientation is a savegame incompatibility issue but I haven't found a solution yet. I can also reproduce the weird lighting issues and I am working on it.

Fixed; see next comment

~~Because phoenix doesn't blindly convert textures to TGA, I have to manually construct a Tempest::Pixmap and memcpy texture data into it. This works totally fine with vanilla Gothic but breaks when loading L'Hiver. Somehow the texture gets deallocated somewhere before ObjectsBucket::uboSetCommon is called which results in a dereference of already freed memory here (https://github.com/lmichaelis/OpenGothic/blob/9f9fca9f58ee8a15d85eae85d35500566113dc55/game/graphics/objectsbucket.cpp#L319). The code that's allocating the texture in Resources::implLoadTexture looks like this:~~

auto rgba = tex.as_rgba8(0);

try {
  Tempest::Pixmap pm(tex.width(), tex.height(), Tempest::Pixmap::Format::RGBA);
  std::memcpy(pm.data(), rgba.data(), rgba.size());
  
  std::unique_ptr<Texture2d> t{new Texture2d(dev.texture(pm))};
  Texture2d* ret=t.get();
  cache[std::move(name)] = std::move(t);

  return ret;
} catch (...) {}

---crashlog(SIGSEGV)---
GPU: AMD RADV SIENNA_CICHLID
#1: Tempest::Detail::VTexture::view(Tempest::ComponentMapping const&, unsigned int) - /mnt/projects/de.lmichaelis/OpenGothic/build/release/opengothic/libTempest.so(_ZN7Tempest6Detail8VTexture4viewERKNS_16ComponentMappingEj+0x17) [0x7f99d8d98ee7]
#2: Tempest::Detail::VDescriptorArray::set(unsigned long, Tempest::AbstractGraphicsApi::Texture*, Tempest::Sampler const&, unsigned int) - /mnt/projects/de.lmichaelis/OpenGothic/build/release/opengothic/libTempest.so(_ZN7Tempest6Detail16VDescriptorArray3setEmPNS_19AbstractGraphicsApi7TextureERKNS_7SamplerEj+0x1d1) [0x7f99d8d8c701]
#3: ObjectsBucket::uboSetCommon(ObjectsBucket::Descriptors&, Material const&) - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN13ObjectsBucket12uboSetCommonERNS_11DescriptorsERK8Material+0x10f) [0x5644d1a8f3ef]
#4: ObjectsBucketDyn::setupUbo() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN16ObjectsBucketDyn8setupUboEv+0x3e) [0x5644d1a8f79e]
#5: VisualObjects::setupUbo() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN13VisualObjects8setupUboEv+0x2d) [0x5644d1aaef6d]
#6: WorldView::setGbuffer(Tempest::Texture2d const&, Tempest::Texture2d const&, Tempest::Texture2d const&, Tempest::Texture2d const&, Tempest::Texture2d const**, Tempest::Texture2d const&) - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN9WorldView10setGbufferERKN7Tempest9Texture2dES3_S3_S3_PPS2_S3_+0x10e) [0x5644d1ab0fee]
#7: Renderer::prepareUniforms() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN8Renderer15prepareUniformsEv+0x136) [0x5644d1a9dd36]
#8: MainWindow::onWorldLoaded() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN10MainWindow13onWorldLoadedEv+0xfc) [0x5644d1ab22dc]
#9: Gothic::finishLoading() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN6Gothic13finishLoadingEv+0xf0) [0x5644d1a3c380]
#10: MainWindow::tick() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN10MainWindow4tickEv+0x155) [0x5644d1ab4a85]
#11: MainWindow::render() - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(_ZN10MainWindow6renderEv+0x85) [0x5644d1ab8125]
#12: Tempest::X11Api::implProcessEvents(Tempest::SystemApi::AppCallBack&) - /mnt/projects/de.lmichaelis/OpenGothic/build/release/opengothic/libTempest.so(_ZN7Tempest6X11Api17implProcessEventsERNS_9SystemApi11AppCallBackE+0x7e) [0x7f99d8dae05e]
#13: Tempest::X11Api::implExec(Tempest::SystemApi::AppCallBack&) - /mnt/projects/de.lmichaelis/OpenGothic/build/release/opengothic/libTempest.so(_ZN7Tempest6X11Api8implExecERNS_9SystemApi11AppCallBackE+0x1c) [0x7f99d8dad76c]
#14: main - /mnt/projects/de.lmichaelis/OpenGothic/build/debug-release/opengothic/Gothic2Notr(main+0x1c6) [0x5644d197cb66]

~~Interestingly enough, the only texture loaded this way is NW_MISC_ROOF_01-C.TEX which is properly decoded by phoenix (original: NW_MISC_ROOF_01-C.TEX.zip). All other vanilla Gothic textures loaded in this way work fine, it's just that one texture of L'Hiver which doesn't work. To keep this branch working for @matthiakl I've added a special check which we should remove after we've found the issue. Do you have any ideas for this @Try?~~

lmichaelis avatar Sep 16 '22 18:09 lmichaelis

Never mind, I've found the issue. It was caused by a value in the cache being overwritten and thus invalidating the previous value. @matthiakl I've also managed to fix the weird sword and door rotations (of course it was related to reading and writing structures to files directly). The colors are also now fixed :)

lmichaelis avatar Sep 17 '22 07:09 lmichaelis

Assuming I would also give this build a try, what kind of issues I should report here? I suspect, not every single spotted issue which likely exist in base OpenGothic but something (what exactly?) specific for this migration. Or the point is playing this one, get some issue, check that ZenLib doesn't have it, and only then report (which sounds quite tedious)?

Dehela avatar Sep 17 '22 10:09 Dehela

Hi @Dehela, it would be ideal to check whether there is already an issue about it in the OpenGothic repo. I assume you have used OpenGothic before though so you'd probably have a basic idea of the kinds of issues are already present. Feel free to report everything you think might be related to this PR and I'll figure out if that's actually the case. It would be nice if you could bundle the issues up into one comment as to not spam the feed though :)

lmichaelis avatar Sep 17 '22 10:09 lmichaelis

I do some runs of ZenLib and there is plenty of issues (to the point that for now I skip reporting some which might be minor or possibly nitpicking) and currently was skipping reporting graphical ones because I had an impression that phoenix version is mostly about visuals (but not sure anymore) so I would wait for it to be merged with master first. If the change ZenLib -> phoenix is blur in sense of knowing what it affects, then it might be better for me to skip it. Also, saves not being backwards compatible makes comparing complicated (you can't load phoenix save in ZenLib to confirm that issue is unique to the former).

By the way, the issue of missing SFXs (like smithing, opening doors, probably more) is present in ZenLib version as well.

Dehela avatar Sep 17 '22 10:09 Dehela

Hello again, I've paused pushing updates to this branch as well as making breaking changes in phoenix so there is finally some room for review. I'm maintaining a conflict-free version of this PR locally which I can push whenever necessary.

CI doesn't fully work right now because AppVeyor's version of MinGW has a broken std::filesystem implementation. As far as I can tell the version they're using was released late 2020 and uses a broken version of GCC. The solution seems to be to use MSYS2, which is also available on AppVeyor, to install MinGW.

lmichaelis avatar Oct 02 '22 18:10 lmichaelis

Hi, @lmichaelis !

Review in progress, meanwhile just overall comment: auto* sf = vm.find_symbol_by_index(func);

Since now find_symbol_by_index can return nullptr this makes old code non-robust and requires null-checking. I'll mark what will be able to spot on review.

Try avatar Oct 03 '22 19:10 Try

One more thing: In code it seems that std::string is core class in VM, what is not ideal. Daedalus string are immutable, similar to Java. Currently at ZenLib I'm using wrapper of shared_ptr<string>, to have fast copy/move operation. Also helps with AiQueue performance, by avoiding extra copies.

Try avatar Oct 03 '22 19:10 Try

Review in progress, meanwhile just overall comment: auto* sf = vm.find_symbol_by_index(func);

Since now find_symbol_by_index can return nullptr this makes old code non-robust and requires null-checking. I'll mark what will be able to spot on review.

Cool, I'll take a look too :)

In code it seems that std::string is core class in VM, what is not ideal. Daedalus string are immutable, similar to Java. Currently at ZenLib I'm using wrapper of shared_ptr, to have fast copy/move operation. Also helps with AiQueue performance, by avoiding extra copies.

The VM does avoid copies internally and basically always passes references (you can see that even externals receive a std::string_view). symbol::get_string returns a const std::string& too. We could probably have a debate about whether the occasional string copy is better or worse than having to reach through two pointers into the heap instead of one, but since I've not noticed any performance degradation at all it might not even be worth talking about.

If we can find a problem with this implementation I'm more than happy to figure out something else but it seems fine so far :) I'll be writing proper benchmarks too so I can more easily profile phoenix. That should help with finding performance issues.

lmichaelis avatar Oct 03 '22 19:10 lmichaelis

About *Definitions classes, for example SoundDefinitions: std::unordered_map<std::string, std::shared_ptr<phoenix::c_sfx>> sfx;

Should it be stored by value? There is little value of keeping connection to VM, as it will be tear-down once hash-map is filled up. Also it makes operator[] awkward: need to check iterator for .end() and then later for nullptr.

Try avatar Oct 03 '22 19:10 Try

Should it be stored by value? There is little value of keeping connection to VM, as it will be tear-down once hash-map is filled up. Also it makes operator[] awkward: need to check iterator for .end() and then later for nullptr.

Hm storing it by value would be fine though it would impact load times, especially with sounds since there are so many of them.

Your question actually uncovers something else I haven't thought about yet: the second null-check in operator[] would not be necessary since the instance pointer is guaranteed to be non-null if init_instance exits properly. Should anything fail inside the VM, a script_error (or a specialized vm_exception) is thrown. That actually means, that there should probably be try-catch blocks wherever init_instance is called, otherwise loading would just fail and the user would be thrown back into the main menu.

lmichaelis avatar Oct 03 '22 20:10 lmichaelis

The VM does avoid copies internally and basically always passes references

case opcode::movs -> set_string

While not 100% repeatable, I've notice performance difference a while ago, due to talking npc in town - they calling B_Say(self,self,"$SMALLTALK00"); in a loop, to emit background talks. It would be shame to make copy for any of those calls.

init_instance exits properly. Should anything fail inside the VM, a script_error (or a specialized vm_exception) is thrown.

That make sense for fail-fast strategy. But for game, should we try-catch any all init calls? so returning null is generally fine with me, but need to de-null value as soon as possible.

otherwise loading would just fail and the user would be thrown back into the main menu

What about in-game calls for invalid script? In NOTR, pirate Samuel has invalid items in his trade menu, for example.

Try avatar Oct 03 '22 21:10 Try