Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: resolve minimap tile urls in map component #59

Merged

Conversation

fallenoak
Copy link
Member

This PR removes the redirect mechanism to resolve minimap tile URLs in favor of a tile index API that serves up the mapping of tile path to content path for each tile directory in the trs file.

The immediate benefit is fewer HTTP requests when rendering minimaps in spelunker-web. The next phase of this work will involving switching to @wowserhq/format's Blp class to render tile .blp files directly in spelunker-web.

@fallenoak fallenoak merged commit e8b6a74 into master Dec 2, 2023
@fallenoak fallenoak deleted the feat-minimap-resolve-tile-urls-in-minimap-component branch December 2, 2023 21:45
Copy link
Member

@timkurvers timkurvers left a comment

Choose a reason for hiding this comment

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

🥳

const [tilePath, contentPath] = line.split(/\t+/);
const tilePathParts = tilePath.split(/[/\\]/);
const tileDirectory = tilePathParts.slice(0, -1).map((t) => t.toLowerCase()).join('/');
const tileName = tilePathParts.at(-1).split('.')[0].toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure but have a feeling we may be able to piggyback on path.posix or path.win32 here to do the heavy lifting of converting between one and the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to recall if MPQs are path delimiter agnostic or not. If not, I'd agree that we could probably use path.win32.parse to extract the directory and file name. Regardless, I suspect the trs file is always written using \, so path.win32.parse likely always works.

res.sendStatus(404);
}
});

Copy link
Member

Choose a reason for hiding this comment

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

👍

return `${process.env.PIPELINE_URI}/minimap/${mapName}/${tx}/${ty}.blp.png`;

// TODO use real url
const unknownTileUrl = `${process.env.PIPELINE_URI}/files/textures/minimap/unknown_${tx}_${ty}`;
Copy link
Member

Choose a reason for hiding this comment

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

Given the TODO here, is this a 404 at present? Could this be a generic URL that does not need tx and ty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is an intentional 404. The tx and ty is present just to make it easier to trace while we're monkeying a bunch with GameMap.

Do you know if Leaflet accepts null responses from getTileUrl? I haven't looked too closely yet.

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