!nrWTeiJJWRzqMEWwAy:matrix.org

OpenMW Development

745 Members
OpenMW project channel. Discussion of topics loosely related to the project development. || Main channel:  #openmw:matrix.org5 Servers

Load older messages


SenderMessageTime
1 Aug 2021
@_discord_713197523526746122:t2bot.iocc9cii So, I spent the last little while trying to implement the "one reader" for ESM3 and ESM4 file types. I thought about placing this note in [Issue #6176](https://gitlab.com/OpenMW/openmw/-/issues/6176) but I don't think implementation notes belong in Issues. Probably best to put such in an MR, but I'm not sure there should be an MR to begin with.

The TL;DR is that the whole premise, i.e. "one reader", may not make sense.

Why one reader?
I would imagine the basis for wanting "one reader" would be to reduce code duplication and to ease future maintenance. Why maintain two if you can maintain just one?

What are the duplicated/common code blocks?
Surprisingly, there isn't much. The file/stream open() and close() methods, and low level data access.

What are the differences?
There are many.
* ESM3 or ESM4 header for the file/stream
* file context structure (e.g. ESM4 context require more info)
* record/sub-record header sizes and structures (which are required to get the next record/sub-record)
* navigating through the records (e.g. the concept of nested "groups" in ESM4)
* fail() method to report errors (different structures require different implementation)

Are there any costs in implementing one reader?
Performance - without resorting to rather complicated set o template classes, we end up with vtable lookups in many of the often used method calls. Note complicated templates will hinder the original aim of ease of maintenance. Note2: profiling has not been done, so I'm unable to quantify the performance hit at this point.

Complexity - even with the introduction of a base class (and resulting vtables) we can't avoid complexity with template methods - e.g. to get/set file context for the different ESM types

So what are the alternatives?
Continued below due to text limit.
01:03:09
@_discord_713197523526746122:t2bot.iocc9cii The obvious alternative is to have two separate readers.

So don't do anything?
Not quite..
* There is still scope to increase maintainability. The two readers that currently exist are written in different style and the way the readers are used (i.e. the API) are different.
* It should be possible to reduce code duplication by declaring some methods in anonymous namespace (suspect there will be many different ways to do this).
01:12:29
@_discord_713197523526746122:t2bot.iocc9cii The obvious alternative is to have two separate readers.

So don't do anything?
Not quite. We can still meet some of the original aim:
* There is still scope to increase maintainability. The two readers that currently exist are written in different style and the way the readers are used (i.e. the API) are different.
* It should be possible to reduce code duplication by declaring some methods in anonymous namespace (suspect there will be many different ways to do this).
01:13:48
@_discord_713197523526746122:t2bot.iocc9cii The obvious alternative is to have two separate readers.

So don't do anything?
Not quite. We can still meet some of the original aim:
* There is still scope to increase maintainability. The two readers that currently exist are written in different style and the way the readers are used (i.e. the API) are different.
* It should be possible to reduce code duplication by declaring some methods in anonymous namespace (suspect there will be many different ways to do this).

Which reader then?
Well, I can't be objective here.
01:14:43
@_discord_471429575919009802:t2bot.iojvoisin#8882 Implementation notes might belong in issues 01:15:28
@_discord_471429575919009802:t2bot.iojvoisin#8882 Instead of being lost in the discord limbo :) 01:15:38
@_discord_713197523526746122:t2bot.iocc9cii maybe I park it there until an MR is raised? 01:16:51
@_discord_471429575919009802:t2bot.iojvoisin#8882 Sure 01:17:05
@_discord_471429575919009802:t2bot.iojvoisin#8882 We can always edit the issue :) 01:17:22
@_discord_235119194323025920:t2bot.ioPeleus joined the room.01:18:02
@_discord_713197523526746122:t2bot.iocc9cii I copied above note to Issue #6176. 01:22:29
@_discord_713197523526746122:t2bot.iocc9cii The obvious alternative is to have two separate readers.

So don't do anything?
Not quite. We can still meet some of the original aim:
* There is still scope to increase maintainability. The two readers that currently exist are written in different style and the way the readers are used (i.e. the API) are different. By having a common (and possibly more modern C++) we can increase maintainability.
* It should be possible to reduce code duplication by declaring some methods in anonymous namespace (suspect there will be many different ways to do this).

Which reader then?
Well, I can't be objective here.
01:23:19
@_discord_713197523526746122:t2bot.iocc9cii * So, I spent the last little while trying to implement the "one reader" for ESM3 and ESM4 file types. I thought about placing this note in [Issue #6176](https://gitlab.com/OpenMW/openmw/-/issues/6176) but I don't think implementation notes belong in Issues. Probably best to put such in an MR, but I'm not sure there should be an MR to begin with.

The TL;DR is that the whole premise, i.e. "one reader", may not make sense.

Why one reader?
I would imagine the basis for wanting "one reader" would be to reduce code duplication and to ease future maintenance. Why maintain two if you can maintain just one?

What are the duplicated/common code blocks?
Surprisingly, there isn't much. The file/stream open() and close() methods, and low level data access.

What are the differences?
There are many.
* ESM3 or ESM4 header for the file/stream
* file context structure (e.g. ESM4 context require more info)
* record/sub-record header sizes and structures (which are required to get the next record/sub-record)
* navigating through the records (e.g. the concept of nested "groups" in ESM4)
* fail() method to report errors (different structures require different implementation)

Are there any costs in implementing one reader?
Performance - without resorting to rather complicated set o template classes, we end up with vtable lookups in many of the often used method calls. Note complicated templates will hinder the original aim of ease of maintenance. Note2: profiling has not been done, so I'm unable to quantify the performance hit at this point.

Complexity - even with the introduction of a base class (and resulting vtables) we can't avoid complexity with template methods - e.g. to get/set file context for the different ESM types

So what are the alternatives?
Continued below due to text limit.
01:25:27
@_discord_713197523526746122:t2bot.iocc9cii * The obvious alternative is to have two separate readers.

So don't do anything?
Not quite. We can still meet some of the original aim:
* There is still scope to increase maintainability. The two readers that currently exist are written in different style and the way the readers are used (i.e. the API) are different. By having a common (and possibly more modern C++) we can increase maintainability.
* It should be possible to reduce code duplication by declaring some methods in anonymous namespace (suspect there will be many different ways to do this).

Which reader then?
Well, I can't be objective here.
01:30:14
@_discord_178356627139854347:t2bot.ioCMAugust#6575 I've heard a report of stuttering on crossing a cell that is present in latest builds but not 0.47.0, even when object paging is turned off. Is that the case for anyone else? 01:56:33
@_discord_599630931216695297:t2bot.ioeddie5 Has been. IDK about 0.47. And I can't even say that that it makes the load take longer. 01:58:30
@_discord_178356627139854347:t2bot.ioCMAugust#6575 The user in question has a weaker PC than mine, so I'm having trouble reproducing it on my end. 02:02:30
@_discord_178356627139854347:t2bot.ioCMAugust#6575 Update: unchecking navmesh generation in the launcher fixed his stuttering. 02:03:17
@_discord_599630931216695297:t2bot.ioeddie5 I haven't noticed it so much since I set the distance from player or whatev to 1 or 0 - I forget. 02:04:14
@_discord_599630931216695297:t2bot.ioeddie5 1. 02:04:49
@_discord_263304948291207170:t2bot.iodexasz#6961 changed their profile picture.02:30:54
@_discord_863996108535365673:t2bot.ioIsaiah 1 11#7054 Depends I heard some SHOTN stuff can cause navmesh generation to get a little…funky 02:53:34
@_discord_456454042441351178:t2bot.ioArbok joined the room.07:00:11
@_discord_777709614329757696_=44arth=47andalf:t2bot.io_discord_777709614329757696_=44arth=47andalf cc9cii Implementation notes might belong in code comments 07:21:16
@_discord_713197523526746122:t2bot.iocc9cii you're probably right, but we don't do that (as in, notes such as above) in our codebase, - and in any case I'm explaining why I'm not writing some code so there's no convenient place to put the notes 07:23:10
@_discord_713197523526746122:t2bot.iocc9cii * you're probably right, but we don't do that (as in, notes such as above) in our codebase, - and in any case I'm explaining why I'm not writing some code so there's no convenient place to put the notes 07:27:22
@_discord_332615662751186945:t2bot.ioSinscere joined the room.08:11:40
@_discord_127352532384088064:t2bot.iopsi29a#3708 Replied with an alternative path 09:05:57
@_discord_713197523526746122:t2bot.iocc9cii Thanks, I also replied, since I didn't quite understand the alternative 09:26:32
@_discord_127352532384088064:t2bot.iopsi29a#3708 Good thing we're talking it out 😉 10:33:52

There are no newer messages yet.


Back to Room List