Added command line utility to test out the library interface #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a command line utility to test out the library interface Added include directives to camera_options.go to allow it to find c/c++ libraries.
gdey
commented
Aug 26, 2018
| img = ss.Snapshot(&FBounds, size) | ||
|
|
||
| case CmdTile: | ||
| img = mbgl.SnapshotTile(ss, FTile, size) |
There was a problem hiding this comment.
@ear7h I think instead of size, which is difficult to understand what is going on here. Since you are setting the X, Y for width and height. I think instead you should just take a width and height parameter of type uint.
There was a problem hiding this comment.
Also, these should return errors.
There was a problem hiding this comment.
Errors need to be implemented on the C side. #1 is tracking this and I currently put in a suggestion for this using macros
|
@ear7h I only have one comment. Not much of any api really which is good. We need to improve the documentation quite a bit. |
|
I need some help testing this as I get the following errors: For |
|
The second issue is related to #11. Not sure what to do about the style out of date problem |
ear7h
reviewed
Aug 26, 2018
mbgl/camera_options.go
Outdated
|
|
||
| /* | ||
| #cgo CXXFLAGS:-I${SRCDIR}/../mapbox-gl-native/include | ||
| #cgo CXXFLAGS:-I${SRCDIR}/../mapbox-gl-native/platform/default |
There was a problem hiding this comment.
Could you move these to mbgl.go, I want to keep the compiler directives in one file to easily keep track of them.
There was a problem hiding this comment.
Sure, we will need to have these things be on a per OS bases soon. I did this just to get this out.
There was a problem hiding this comment.
It is currently split up by OS (see mbgl_darwin.go and mbgl_linux.go) although, I haven't tested the bindings on linux.
| img = ss.Snapshot(&FBounds, size) | ||
|
|
||
| case CmdTile: | ||
| img = mbgl.SnapshotTile(ss, FTile, size) |
There was a problem hiding this comment.
Errors need to be implemented on the C side. #1 is tracking this and I currently put in a suggestion for this using macros
cmd/snapshot/main.go
Outdated
| for i, part := range parts { | ||
| switch i { | ||
| case 0: | ||
| v, err = strconv.ParseUint(strings.TrimSpace(part), 10, 64) |
There was a problem hiding this comment.
Could this be outside of the switch statement?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a command line utility to test out the library interface
Added include directives to camera_options.go to allow it to
find c/c++ libraries.