-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
d6d17e4
to
32615d4
Compare
cmake/core-files.cmake
Outdated
src/mbgl/renderer/tile_parameters.hpp | ||
src/mbgl/renderer/tile_pyramid.cpp | ||
src/mbgl/renderer/tile_pyramid.hpp | ||
src/mbgl/renderer/transitioning_property.hpp | ||
src/mbgl/renderer/update_parameters.hpp | ||
|
||
# renderer/buckets |
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.
👍 on subdirectories for buckets and layer. Can you submit that as an independent PR so that it's easily reviewable independently? Will also help minimize potential merge conflicts as these files are getting touched a lot right now.
namespace conversion { | ||
|
||
template<> | ||
struct Converter<std::unique_ptr<LatLng>> { |
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.
No need for unique_ptr
here; LatLng
is a value class.
include/mbgl/util/geo.hpp
Outdated
@@ -47,7 +47,7 @@ class LatLng { | |||
wrap(); | |||
} | |||
} | |||
|
|||
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.
Nit: whitespace change.
@@ -107,6 +107,7 @@ OfflineRegionStatus OfflineDownload::getStatus() const { | |||
|
|||
case SourceType::Video: | |||
case SourceType::Annotations: | |||
case SourceType::Image: | |||
break; |
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.
We'll need to add offline support for image sources -- it should download and store the image in the offline database.
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.
Offline support added in 3506bb5
@@ -176,7 +177,8 @@ void OfflineDownload::activateDownload() { | |||
|
|||
case SourceType::Video: | |||
case SourceType::Annotations: | |||
break; | |||
case SourceType::Image: | |||
break; |
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.
Whitespace.
cmake/core-files.cmake
Outdated
@@ -74,6 +74,8 @@ set(MBGL_CORE_FILES | |||
src/mbgl/gl/object.hpp | |||
src/mbgl/gl/primitives.hpp | |||
src/mbgl/gl/program.hpp | |||
src/mbgl/gl/program_binary.cpp | |||
src/mbgl/gl/program_binary.hpp |
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.
Bogus file add
} | ||
//Style spec uses GeoJSON convention for specifying coordinates | ||
optional<float> latitude = toNumber(arrayMember(value, 1)); | ||
optional<float> longitude = toNumber(arrayMember(value, 0)); |
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.
These can be const
|
||
std::vector<std::unique_ptr<LatLng>> coordinates; | ||
coordinates.reserve(4); | ||
for( std::size_t i=0; i < arrayLength(*coordinatesValue); i++) { |
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.
Whitespace: We're using for (std::size_t i = 0; i < arrayLength(*coordinatesValue); i++) {
return {}; | ||
} | ||
|
||
auto result = std::make_unique<ImageSource>(id, *urlString); |
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.
We could move the string into the constructor
return impl->getURL(); | ||
} | ||
|
||
} // namespace style |
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.
Nit: We're not indenting namespaces
src/mbgl/renderer/render_item.hpp
Outdated
const RenderLayer& layer; | ||
std::vector<std::reference_wrapper<RenderTile>> tiles; | ||
RenderLayer& layer; | ||
RenderSource * source; |
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.
Whitespace: RenderSource* source
;
: RenderSource(impl_), | ||
impl(impl_), | ||
loaded(false) { | ||
} |
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.
Formatting
const mat4& clipMatrix, | ||
const TransformState&) final; | ||
|
||
void render (Painter&, PaintParameters& , const RenderLayer& ) ; |
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.
Whitespace ;)
return url; | ||
} | ||
|
||
void ImageSource::Impl::setCoordinates(const std::vector<LatLng> coords_) { |
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.
This should be either a non-const value (to enable move), or a const &
to avoid two copies.
} else { | ||
return {}; | ||
} | ||
return {}; |
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.
Unreachable code
8da5ce5
to
ffff1c6
Compare
|
||
class ImageSource : public Source { | ||
public: | ||
ImageSource(std::string id, const std::vector<LatLng>); |
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.
How come this isn't a LatLngBounds
? Similarly, it's very hard to discern the coordinate order this way.
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.
Apparently an image source can be associated with any quadrilateral, not necessarily a rectangle.
mbgl tends to use std::array
to represent fixed-length collections in style values (cf. padding and offsets). As @incanus points out, a struct with named fields would be another option that would avoid corner-swapping problems, although maybe we could make it so that the input order doesn’t matter.
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.
@incanus - thats an excellent point. Ive tried to wrap around that in the SDKs in the draft API PR. If the Quad struct makes sense, I think it might make more sense to move it into native.
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.
Note that there would have to be separate types for mbgl and for the iOS/macOS SDK, because the iOS/macOS SDKs can’t expose any C++ to the public.
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.
mbgl tends to use
std::array
to represent fixed-length collections
Correct -- the type should be std::array<LatLng, 4>
.
Quick & dirty iOS bindings for a near-term use case I have, per chat with @asheemmamoowala: |
ffff1c6
to
ffad32a
Compare
ffad32a
to
6d940a9
Compare
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.
Getting close! In addition to the changes requested below, please enable the relevant integration tests in mapbox-gl-js, open a PR there, and update the submodule pin here.
template <class V> | ||
optional<LatLng> operator() (const V& value, Error& error) const { | ||
if (!isArray(value) || arrayLength(value) < 2 ) { | ||
error = { "coordinate array must contain numeric longtitude and latitude values" }; |
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.
Typo for "longitude".
|
||
std::vector<LatLng> coordinates; | ||
coordinates.reserve(4); | ||
for( std::size_t i=0; i < arrayLength(*coordinatesValue); i++) { |
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.
Nit: whitespace. Also can use fixed upper bound. for (std::size_t i = 0; i < 4; i++)
|
||
class ImageSource : public Source { | ||
public: | ||
ImageSource(std::string id, const std::vector<LatLng>); |
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.
mbgl tends to use
std::array
to represent fixed-length collections
Correct -- the type should be std::array<LatLng, 4>
.
@@ -66,6 +66,15 @@ inline optional<float> toNumber(const mbgl::android::Value& value) { | |||
} | |||
} | |||
|
|||
inline optional<double> toDouble(const mbgl::android::Value& value) { | |||
if (value.isNumber()) { | |||
auto num = value.toDouble(); |
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.
return value.toDouble();
ImageSource(std::string id, const std::vector<LatLng>); | ||
~ImageSource() override; | ||
|
||
const std::string& getURL() const; |
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.
Return type should be optional<std::string>
, matching {Vector,Raster}Source::getURL
and reflecting the possibility of absence.
setupBucket(geomCoords); | ||
} | ||
|
||
void RenderImageSource::setupBucket(GeometryCoordinates& geomCoords) { |
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.
This is only called from updateTiles
-- inline it there.
@@ -0,0 +1,71 @@ | |||
#pragma once | |||
|
|||
#include <mbgl/renderer/buckets/raster_bucket.hpp> |
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.
Forward declare RasterBucket
(and define ~RenderImageSource
in the .cpp file) instead.
#include <mbgl/renderer/render_source.hpp> | ||
#include <mbgl/renderer/render_tile.hpp> | ||
#include <mbgl/style/sources/image_source_impl.hpp> | ||
#include <mbgl/tile/geometry_tile_data.hpp> |
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.
Unused.
#include <mbgl/renderer/render_tile.hpp> | ||
#include <mbgl/style/sources/image_source_impl.hpp> | ||
#include <mbgl/tile/geometry_tile_data.hpp> | ||
#include <mbgl/util/image.hpp> |
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.
Unused.
#include <mbgl/style/sources/image_source_impl.hpp> | ||
#include <mbgl/tile/geometry_tile_data.hpp> | ||
#include <mbgl/util/image.hpp> | ||
#include <mbgl/util/optional.hpp> |
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.
Unused.
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 to merge once CI is passing. The current failure is:
==12458== Invalid write of size 4
==12458== at 0x5D79AF: std::vector<unsigned int, std::allocator<unsigned int> >::push_back(unsigned int const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)
==12458== by 0x5D7872: mbgl::gl::detail::TextureDeleter::operator()(unsigned int) const (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)
==12458== by 0x49B1BC: mbgl::RasterBucket::~RasterBucket() (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)
==12458== by 0x498189: Buckets_RasterBucket_Test::TestBody() (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)
Which indicates that in the RasterBucket
test, the declaration order for the bucket and the context should be switched, so that the context is still alive when the bucket is destroyed.
989918b
to
91f2a9b
Compare
Add support for ImageSource to mbgl-core. The current source passes existing render tests and can be verified with the macos app/mbgl-glfw.
Remaining Tasks
iOS/Android/Qt Bindings ? ⛷(Working on a separate PR)//cc @jfirebaugh @ivovandongen @kkaefer