ZenLib icon indicating copy to clipboard operation
ZenLib copied to clipboard

MdsParser design proposal

Open Try opened this issue 6 years ago • 3 comments

Hi, I have an idea how to make Mds parser better.

Problem

In current design we have ModelScriptBinParser/ModelScriptTextParser with base class ModelScriptParser. Every time someone want's to add support for new chunk-type, code has to be written twice: once for Bin, once for Txt.

Possible solution

Move more logic into ModelScriptParser base class. It can be done, by introducing a new group of virtual functions, like:

      virtual std::string readStr()=0;
      virtual std::string readKeyword()=0;
      virtual uint32_t    readDWord()=0;
      virtual int32_t     readI32()=0;
      virtual float       readFloat()=0;
      ... 

Prototype

Here is a prototype, I have in my fork: https://github.com/Try/ZenLib/blob/master/zenload/modelScriptParser.cpp Testing: run locally Gothic2Notr, Gothic2, Gothic1, Gothic reloaded. Test suite: https://github.com/Try/ZenLib/blob/master/tests/test_mds.cpp (sorry, but no test-data in repo - I've had to use dumps from game assets)

Benefits

  • Less code
  • Feature-consistency over gothic1-2
  • Write once - debug everywhere king of code

Cons

  • Debug everywhere king of code, because one line can change behavior for both formats.
  • CHUNK_MESH_AND_TREE doesn't match between txt and bin formats - implemented as virtual function.
  • Prototype contains unrelated changes(more chunks + removal of get-function in favor of public access)
  • CHUNK_ERROR handling not implemented in prototype - text parser is always relaxed.

Andre, welcome back by the way :)

Try avatar Nov 04 '19 19:11 Try

I haven't thought of that, but it seems like a great idea!

I'll look into it more closely tomorrow when I have more than my phone.

ataulien avatar Nov 04 '19 21:11 ataulien

Sorry for taking so long. The new design looks really nice! You even managed to half the length of the source file!

I'm not so worried about the cons you listed though, the benefits outweigh them by a lot IMHO.

ataulien avatar Nov 14 '19 21:11 ataulien

OK) But now we need to make a plan: how to merge this change in original ZenLib. The trouble:

  • my fork diverted very much from original
  • change is very large for merge: blind cherry-pick - can break things by accident

So, what we can do:

  1. I can clone regoth+vanilla version of ZenLib to prepare proper pull-request( gonna take some time)
  2. You can cherry-pick commit and do alignment, by yourself

Try avatar Nov 17 '19 19:11 Try