Skip to content

Use pos from GameDataLoader#55

Open
morganchristiansson wants to merge 1 commit into
Return-To-The-Roots:masterfrom
morganchristiansson:gdl-pos
Open

Use pos from GameDataLoader#55
morganchristiansson wants to merge 1 commit into
Return-To-The-Roots:masterfrom
morganchristiansson:gdl-pos

Conversation

@morganchristiansson

Copy link
Copy Markdown
Contributor

Instead of current hardcoded values.
GameDataLoader loads them from data/RTTR/gamedata/world/*.lua

This also removes water animations as they use coordinate offsets rather than palette animation, which should be addressed in another PR.

@morganchristiansson morganchristiansson changed the title GameDataLoader pos Use pos from GameDataLoader Jun 27, 2026

@Flamefire Flamefire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looses the correctly animated textures which is actually missing in s25main. The editor is currently the only instance that (still) handles it correctly so we won't remove it and make it look worse.

And shouldn't we e.g. check maps for "known" textures on load instead of handling that in getTerrainDesc? I'm not sure the handling will still be correct in absence of terrain data.

Comment thread CSurface.cpp
if(s2Id < map.s2IdToTerrain.size())
{
const auto idx = map.s2IdToTerrain[s2Id];
if(idx)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exclude the 0 index?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if(idx) check is correct because idx is a DescIdx, not a plain integer. Its
operator bool() checks value != INVALID (0xFF), so index 0 passes.

@morganchristiansson

Copy link
Copy Markdown
Contributor Author

This looses the correctly animated textures which is actually missing in s25main. The editor is currently the only instance that (still) handles it correctly so we won't remove it and make it look worse.

What is correctly animated textures?

The s25edit water animation currently jumps when repeating the animation. I assumed s25client which uses palette cycling is correct and the animation looks better there.

There is a s25client class that handles animation with palettes, I was hoping to use that but it likely relies on OpenGL so will need to be done later.

@Flamefire

Copy link
Copy Markdown
Member

What is correctly animated textures?

Maybe I misremember but you removed the animation completely. It was based on color keys here and on selecting different parts of the texture.

Seems like no one really knows what is "correct" but no animation certainly is worse than the current behavior.

@morganchristiansson

Copy link
Copy Markdown
Contributor Author

Yes removed the animation it relied on hardcoded values not available through GDL and broke in this PR.
But I think selecting different parts of the texture is a lazy hack, the correct way is to cycle palette like s25client does.

Maybe can try to re-add it as it was before while still using GDL pos for textures.
Or if we can get to the OpenGL PRs where it will be different again and could use s25main class rather than separate implementation.

@morganchristiansson

Copy link
Copy Markdown
Contributor Author

Seems like no one really knows what is "correct" but no animation certainly is worse than the current behavior.

Did noone use ghidra or similar to reverse engineer the original?
There are palette files tho so seems likely that s25client is correct.

I created a branch off this PR to use palette animations for water and lava like s25client.
Will need to be re-done for OpenGL in future branch when we get there.
I can include it in this PR? morganchristiansson/s25edit@gdl-pos...palanim

@Flamefire

Copy link
Copy Markdown
Member

Did noone use ghidra or similar to reverse engineer the original?
There are palette files tho so seems likely that s25client is correct.

Possibly at some point someone did, or just inspected and experimented with the raw files. That was years ago though.

I can include it in this PR? morganchristiansson/s25edit@gdl-pos...palanim

Sure. At some point it might be better to use libsiedler2 for loading, even if just loading into the current structures (surfaces etc) to avoid duplicating even more of the loading/saving code.

@morganchristiansson

Copy link
Copy Markdown
Contributor Author

Sure. At some point it might be better to use libsiedler2 for loading, even if just loading into the current structures (surfaces etc) to avoid duplicating even more of the loading/saving code.

100%. I did consider it for palette animations. There's actually special palette files it seems to load for the animations, so it's probably the correct way.
But s25edit already has CFile that also handles writing..Maybe can add write swd support to libsiedler2 for s25edit.
And it seemed like a lot to pull in for just 1 new format.

I think maybe can do palette anim as separate PR before this PR. Obviously there will be conflicts but I don't mind.

@Flamefire

Copy link
Copy Markdown
Member

But s25edit already has CFile that also handles writing..Maybe can add write swd support to libsiedler2 for s25edit.

I'd say writing maps implemented in the editor is fine, so that could stay there and at least during transition (only) the parsing code of CFile could be replaced by loading an libsiedler2 archive and reading those items into the existing structs. Basically just avoid having to implement the same logic again.
And then replace more as needed.

And it seemed like a lot to pull in for just 1 new format.

My thinking would be: We need information from the file we currently don't have, so instead of duplicating more parsing code here that would need testing etc. use what we have, just for this part. It might be easier to read the whole file with libsiedler2 causing slightly larger changes but still only affects a small part of CFile without touching much else. If the other parsing code works, why touch it?
Personally I don't like the approach in s25client with a parallel type-hierarchy, and would rather load the archive and transform in something usable then and there, similar to what CFile partially does. Much easier here as it deals with much less variations of files and archive items.
But well, historical code 🤷

@morganchristiansson

morganchristiansson commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Also there's the issue that s25edit stores in-memory USD triangles on even rows at an offset relative to s25client.
I'm addressing in #48

Throws both me and AI into a tailspin trying to grok this and throws a spanner in re-use.

Further to load files with libsiedler2 the data storage types need to be switched to libsiedler2.
I think it's all worth doing but will be a long incremental process.

And I will leave this PR in conflict until #57 is merged so it doesn't remove the water&lava animations.

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.

2 participants