Skip to content

Draft: add y-offset to overlays, store complete pictures. - #29

Draft
Lknadfodr wants to merge 2 commits into
Return-To-The-Roots:masterfrom
Lknadfodr:issue-1966
Draft

Draft: add y-offset to overlays, store complete pictures.#29
Lknadfodr wants to merge 2 commits into
Return-To-The-Roots:masterfrom
Lknadfodr:issue-1966

Conversation

@Lknadfodr

Copy link
Copy Markdown

Draft for Issue 1966
The game runs and the sprites are correct.

However I have no knowledge about the links array and merging them.

@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.

Just thinking out loud as I don't really know the link-stuff either. So please correct me.

I think the biggest change is:

-        set(NUM_BODY_IMAGES + links[i], std::move(image));
+        set(getOverlayIdx(i), std::move(image));

i.e.

-        set(NUM_BODY_IMAGES + links[i], std::move(image));
+        set(NUM_BODY_IMAGES + i, std::move(image));

previously get used archive[NUM_BODY_IMAGES + links[idx]], now we have archive[NUM_BODY_IMAGES + idx]

So that does look the same

With the removed if(loaded[links[i]]) continue I think the above is correct but stores duplicates.
I.e. it looks like bob-idx x and y could previously use the same image z if their links[x] == links[y] == z
Now we store that twice: At x and y instead of only at z but that allows us to set ny directly

I guess that is negligible as we unwrap it during loading.

What we could do is a) remove the links member and b) still keep it local to the method so that when we have already loaded an image we just clone it and adjust the ny instead of loading it from scratch.
Can you check how often this "sharing" actually happens to see if this is worth it?

@Lknadfodr

Copy link
Copy Markdown
Author

I checked the number of overlays and the number of complete pictures, here are the results:

In CARRIER.BOB there are 602 overlays and 3264 complete pictures.
In JOBS.BOB there are 2269 overlays and 8928 complete pictures.

So, loading each overlay just once actually saves quite a lot (~80% for CARRIER, ~75% for JOBS).

While at it, I also counted how many different combinations of overlay-link and ny-correction there are:
In CARRIER.BOB there are 1011 distinct combinations, meaning 2253 duplicates use the same link+ny.
In JOBS.BOB there are 2464 distinct combinations, 6464 are duplicates.

I'll update so it will load each overlay just once.

In general I see two options:

  1. We apply the ny-correction right away. In this case, the archive has the same overlay image multiple times with different ny values. This keeps the change opaque for the rest of the code base.
  2. We do not apply the ny-correction here, but instead offer a new method like getOverlayNyCorrection(overlayIdx, fat, direction, animationstep). Then the caller has to retrieve that seperately and apply it when the full sprite with body+overlay is assembled.

Let me know if you prefer option 2.

@Lknadfodr
Lknadfodr marked this pull request as draft July 31, 2026 13:53
@Flamefire

Copy link
Copy Markdown
Member

Ok so we have multiple dimensions. Let me try to unpack:

  • std::vector<uint16_t> links; /// Array [overlayId][animStep=8][fat=2][direction=6] mapping to an overlay picture
    This is a mapping of the overlay to an image index
  • Each image might be used for multiple overlays but the y-offset might be different

It makes sense to

  1. Load each image exactly once -> reduce loading time
  2. Store each image exactly once -> reduced memory footprint

I'd say we use the original approach similar to option 2:

  • Load each overlay once -> keep links
  • Load y-offsets
  • getOverlay returns a pair of image and y-offset as you always need both, don't you?
  • Rename "links" to e.g. "imageIndex" and update documentation/docstrings to make it easier to understand. Pick something you'd have liked to be able to understand the mechanism. Like "mapping of each overlay (index) to actual bitmap (index) stored in memory"
  • Similar for a global y-offset/ny array to be added

Comment on lines +37 to 38
uint16_t getOverlayIdx(unsigned idx) const;
uint16_t getOverlayIdx(unsigned overlayIdx, bool fat, ImgDir direction, unsigned animationstep) const

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.

I'd say we can inline this into getOverlay

Comment thread src/ArchivItem_Bob.cpp
return ErrorCode::UNEXPECTED_EOF;

if(links[i] >= numOverlayImgs)
if(link >= numOverlayImgs)

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.

I do like having a local variable link even when we keep the links[i] as it makes it easier to read below to not have links[i] all over

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