ZenKit migration issues

[Try]

1. VirtualObject::visual can be null after loading. This makes usage unreasonable complex, forcing extra NPE-check. VParticleEffectController,VMover are among of the cases when both null and non-null may occur.
2. VirtualObject::id is essential part of save files, but now deprecated
3. It seem set(ZK_ENABLE_DEPRECATION OFF CACHE INTERNAL "") has no effect anymore - wasn't able to track why exactly it is

-- 16.04.2024:

// dangling reference to stack-allocated callback
void DaedalusVm::register_default_external(std::function<void(std::string_view)> const& callback) {
    this->register_default_external([callback](DaedalusSymbol const& sym) { callback(sym.name()); });
}

size+1 error in multiple places:

DaedalusSymbol const* DaedalusScript::find_symbol_by_index(std::uint32_t index) const {
    if (index > _m_symbols.size()) { // need to be  >=
        return nullptr;
    }
...
DaedalusSymbol* DaedalusScript::find_symbol_by_index(std::uint32_t index) {
void DaedalusVm::jump(std::uint32_t address) {
// maybe more, those I just stumble upon, while working on my stuff

This is not complete list, rather some immediate observations to start discussion.

[lmichaelis]

Hio @Try ,

I'll investigate point 3 and I'll get back to you on that.

As for point 1: This change has been introduced for better compatibility with the original format and to allow saving the VOb-tree using ZenKit. Internally, I now track zReference objects in the Archives which allows me to properly deduplicate items during save/load but to facilitate it, I had to move the logic for reading archive objects into a central place. To keep the old API you mention would now be inconsistent with how the entire system works. If you want, I can elaborate on that.

Point 2 falls into the same category: As I understand it, the object ID is purely there for resolving references in the archive. It is unreliable, because it's value is not guaranteed to be the same if you were to re-compile the world and it is not used anywhere in the ZenGin to uniquely identify object, as far as I can tell.

If you need to compare objects, you can now just pointer-compare, since all of the reference resolution happens inside ZenKit and you just get a shared_ptr.

[Try]

.1 This causes nullptr chaos and part of code, that use to work now cause crashes. It would be nice to guarantee, those pointer to be non-null. Without non-null game would have to check visual twice: for null + for asset-file existence.

.2

It is unreliable, because it's value is not guaranteed to be the same if you were to re-compile the world and it is not used anywhere in the ZenGin to uniquely identify object, as far as I can tell.

OpenGothic already uses it for save/load purpose;

void Vob::save(Serialize& fout) const {
  fout.setEntry("worlds/",fout.worldName(),"/mobsi/",vobObjectID,"/data");
  fout.write(uint8_t(vobType),pos,local);
  }

Removing objectId, would mean breaking all save files. Also assumption here, that object id is semi-persistent: when changes in editor are minor - fine and, for major level changes, save would be called incompatible.

---

Also kind remainder, that loading time get progressively slower and slower with updates. It can be due to overuse of small-dynamic allocations, but need to investigate.

[Try]

Updated first comment: Bug in register_default_external causes crash in OpenGothic+G1, plus minor bugs

[lmichaelis]

This causes nullptr chaos and part of code, that use to work now cause crashes. It would be nice to guarantee, those pointer to be non-null. Without non-null game would have to check visual twice: for null + for asset-file existence.

Hm, I don't really have a good solution I think. Right now the order of operations would be:

1. Check if visual == nullptr, if it is: abort, the VOb does not have a visual
2. Check the visual type
   • If it's anything other than a decal: look up the model asset corresponding to the visual type
   • If it is a decal, load the texture and process the decal as usual

Since asset files presumably exist if the object has a visual, the "asset not found" case is exceptional and should thus probably be handled separately.

OpenGothic already uses it for save/load purpose;

True, that's a bigger issue than I thought. My proposal would be: change the save files so that the entire VOb tree is saved, then you can simply load it entirely from the save. This is what the original does. It should also improve reliability and could make loading faster, since you no longer have to read in the original world file.

For now though, I'd say: let's keep the deprecated API around for the foreseeable future. I'd still like to mark it as deprecated so as to discourage others from using it but this legacy use-case I'll continue to support for now if that works for you. Same goes for the first point. If you want to continue to use the old API, go for it. Just tell me about it so I don't remove before coordinating with you.

Also kind remainder, that loading time get progressively slower and slower with updates. It can be due to overuse of small-dynamic allocations, but need to investigate.

Yes, performance improvements are on my bucket list (see also #88). It's not a high priority for me right now though.

Bug in register_default_external causes crash in OpenGothic+G1, plus minor bugs

Fixed in 3d67ed5aad17ad16df334c250e838f486fd21420.

[Try]

Hm, I don't really have a good solution I think

Can you provide a dummy visual, and point to it?

My proposal would be: change the save files so that the entire VOb tree is saved

Backward compatibility :(

Side topic: would it be possible to introduce stream-reader at some point? Shame to parse full-world into ram, only to convert it immediately to internal representation.

[lmichaelis]

Can you provide a dummy visual, and point to it?

I could, though I would lock it behind a compile-time setting if that's okay with you.

Side topic: would it be possible to introduce stream-reader at some point? Shame to parse full-world into ram, only to convert it immediately to internal representation.

You can use use zenkit::ReadArchive directly but there are two caveats: You need to process everything outside of VObs manually (e.g. the childs<n> fields and parent-child structure), and you won't reduce memory use because zenkit::ReadArchive needs to keep a reference for each object to supported in-archive references, unless you do everything manually.

You can use zenkit::ReadArchive::read_object to load a supported object from an archive (type is automatically determined and references are resolved), or you can use zenkit::ReadArchive::read_object_begin and zenkit::ReadArchive::read_object_end to either load semi-automatically or manually.

Method	Related Functions	Description
Automatically using read_object	zenkit::ReadArchive::read_object	Load objects from any .ZEN-file fully automatically. Unsuported or wrapped object must be loaded manually (such as the mesh, waynet and outer layers of the VOb-tree in oCWorld) using read_object_{begin,end} and the field readers.
Semi-automatically using read_object_{begin,end}	zenkit::ReadArchive::read_object_begin zenkit::ReadArchive::read_object_end The load function of VOb instances	Load object headers and surrounding items manually using read_object_{begin,end} and load the object bodies using the load() functions of VOb instances. In this case references of objects read by the load() function are still retained and resolved but references to outer VObs are not loaded. Generally this should work fine when not using the SaveGame API.
Manually using read_object_{begin,end} and read_{int,float,string,...}	zenkit::ReadArchive::read_object_begin zenkit::ReadArchive::read_object_end zenkit::ReadArchive::read_string zenkit::ReadArchive::read_int zenkit::ReadArchive::read_float zenkit::ReadArchive::read_byte zenkit::ReadArchive::read_word zenkit::ReadArchive::read_enum zenkit::ReadArchive::read_bool ...	You can manually write code to parse the VObs on your side. Of course this option would require you to re-implement the entire VOb-loading code already present in the load() function of VOb instances, just mapped to your codebase.

You could also just skip the conversion step and use the ZenKit datastructures directly. Since you can write to them, it should be possible to only use OpenGothic's structures for data not already represented in the VOb abstraction.

For a basic example on how to use zenkit::ReadArchive, see https://zk.gothickit.dev/library/api/archive/#reading-from-an-archive. Note that you could replace the innermost if statement (parsing the zCVob-object) with a call to zen->read_object() to receive an automatically parsed VOb.

In all of these cases though, you will have to re-implement the world wrapper parsing yourself, the mesh, BSP-tree and waynet can be parsed on their own. See this:

https://github.com/GothicKit/ZenKit/blob/89af81cc7439696fef3d144d37f4091d4d54f59b/src/World.cc#L98-L201

[lmichaelis]

Can you provide a dummy visual, and point to it?

Done in 18318ec. Please confirm @Try.

Are there any other concerns with the new version? If not, I'll finally do the 1.3 release :)

[Try]

Hi @lmichaelis !

Fix works only partially: visual is still null with zenkit::VMover and some other objects. Probably related to objects with has_visual_object==false

[lmichaelis]

True, I forgot about that :D Updated in 35938c2.

[Try]

Are there any other concerns with the new version? If not, I'll finally do the 1.3 release :)

Building now with ZK_ENABLE_DEPRECATION=ON - still some stuff to cleanup, will notify your once migration is complete

[Try]

1.

C:/Programming/OpenGothic/lib/ZenKit/include/zenkit/ModelAnimation.hh:163:84: note: declared here
  163 |                         ZKREM("renamed to Animation::sample_position_scale") float sample_position_scalar {};
      |                                                                                    ^~~~~~~~~~~~~~~~~~~~~~
C:/Programming/OpenGothic/game/graphics/mesh/animation.cpp: In constructor 'Animation::Sequence::Sequence(const zenkit::MdsAnimation&, std::string_view)':
C:/Programming/OpenGothic/game/graphics/mesh/animation.cpp:197:26: note: synthesized method 'zenkit::ModelAnimation::ModelAnimation()' first required here
  197 |   zenkit::ModelAnimation p;

mingw doesn't like initialization of deprecated variables... changing it to code bellow resolves the issue:

union {
  ZKREM("renamed to Animation::sample_position_min") float sample_position_range_min;
  float sample_position_min {};
  };

2. VirtualObject::id is still marked as deprecated. As mentioned before: opengothic relies on this field to do serialization.

3. Just a minor note: World::load(Read* r, GameVersion version) takes a pointer, imply that r can be null. Better use a reference here instead,