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
layer dropping #113
layer dropping #113
Conversation
benchmarks round 1Benchmarks look good. Mostly want to see no loss in performance with this addition, which these show. Looks like a slight increase, which is hard to believe, so I'll consider that mostly noise for now. No significant performance changes!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me in general, aside from a few questions inline
: z{z0}, | ||
x{x0}, | ||
y{y0}, | ||
data{buffer.Data(), buffer.Length()}, | ||
buffer_ref{Napi::Persistent(buffer)} | ||
buffer_ref{Napi::Persistent(buffer)}, | ||
layers{std::move(layers0)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this move the layers0
argument instead of using a const reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I'm open to ideas here - the clang tidy/format builds told me to use std::move
here, so I'm admittedly just plugging that in. Can you show me how I'd use a const reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was talking about was 05cbd11bd093d, but you are right and I am wrong, and the TileObject
needs to contain its own copy of the vector instead of holding a reference back to the caller's vector, which apparently goes out of scope before it is used. So please ignore this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for the commit + trying it out @ericfischer!
What changed?
layers
option to the tile objects that allows user to specify which layers to keep in the final vector tileHousekeeping
documentation
auto generated docscc @mapbox/tilesets-api @artemp