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

More detailed Version 3 Specification #172

Merged
merged 25 commits into from
Aug 17, 2023

Conversation

DerZade
Copy link
Contributor

@DerZade DerZade commented May 6, 2023

This PR fixes #107, by attempting to introduce a more detailed specification for version 3.

spec/v3/spec.md Outdated Show resolved Hide resolved
spec/v3/spec.md Outdated Show resolved Hide resolved
spec/v3/spec.md Outdated Show resolved Hide resolved
|`5`|`1337`|`42`|`0`|A leaf directory whose first tile has an ID of 5 is located at byte 1337-1378 of the _leaf directories section_|

### 4.2 Encoding
A directory can only be encoded in its entirety. It is not possible to encode a single directory entry by itself.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdon in #107 (comment):

What implies this? An archive that addresses a single tile should be valid, AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values you encode for an entry always depend on the entry that came before that entry. That's why I said that entries can't be encoded by itself.

Yes an archive that addresses a single tile should be valid, but that would have to have a root directory with one entry. At least you would to have to prepend a 1 to indicate the number of entries, that are contained in the directory and at that point it's a directory and not a single entry.

Copy link
Member

Choose a reason for hiding this comment

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

At least you would to have to prepend a 1 to indicate the number of entries, that are contained in the directory and at that point it's a directory and not a single entry.

OK, I see what you mean here. I meant to say that a directory with length 1 is valid; any directory with length > 0 is valid. So I think 4.2 should simply say "A directory must have at least one entry".

Copy link
Contributor Author

@DerZade DerZade Jul 18, 2023

Choose a reason for hiding this comment

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

I'm happy to add something like "must have at least one entry", although it's already mentioned in another chapter.

But I still think we should keep something along the lines of "A directory can only be encoded in its entirety", because I remember that wasn't really clear to me when I first started implementing PMTiles. What I wanted to know then was how the entries are encoded / decoded, but that's just the wrong starting point, since they are always interleaved. So I was trying to clarify that you always have to encode / decode an entire directory to get the single entries. You cannot encode a single entry since all attributes are spread out over the directory.

@DerZade
Copy link
Contributor Author

DerZade commented Jun 13, 2023

@bdon I'd like to get this done this week (or at least next week).

Could you please provide feedback for the changes since your last review (all commits since d3777f0) and take a look at the open comments?

@DerZade DerZade marked this pull request as ready for review July 17, 2023 13:29
@DerZade
Copy link
Contributor Author

DerZade commented Jul 17, 2023

This is finally ready for review.

The only thing I'm still not sure about if we should move the changelog into a own file instead of having it directly in the spec itself.

spec/v3/spec.md Outdated Show resolved Hide resolved
@DerZade DerZade changed the title More detailed Version 3 Specifiction More detailed Version 3 Specification Jul 18, 2023
@bdon bdon merged commit 1aa0931 into protomaps:main Aug 17, 2023
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.

More detailed Specification
2 participants