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

Update MapLibre GL JS to 49bc4ca45 #339

Closed
wants to merge 3 commits into from

Conversation

github-actions[bot]
Copy link

No description provided.

Copy link
Contributor

@atierian atierian left a comment

Choose a reason for hiding this comment

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

I did extensive manual testing for iOS, with both the iosapp sample app in the project and a "real world" app that uses MapLibre GL Native. Testing was done with an iPhone 13 Pro Max with iOS 15.5 and compared to what's currently in main. I was not able to find any issues or regressions.

The scope of this approval is limited to "doesn't break anything in iOS", I haven't tested how this change affects any other platforms. My only request is that you add a brief explanation (for future reference) in the PR comments about why the changes to the vertexOffset and fragmentOffset values are necessary.

@roblabs
Copy link
Collaborator

roblabs commented Jun 20, 2022

My only request is that you add a brief explanation (for future reference) in the PR comments about why the changes to the vertexOffset and fragmentOffset values are necessary.

I have this question, too. Where did the changes come from?

Maybe a link back to the MapLibre GL JS would be useful.

@wipfli
Copy link
Contributor

wipfli commented Jun 23, 2022

The changes to fragmentOffset and vertexOffset are probably due to longer fragment and vertex shaders... This code gets generated here I think:

https://github.com/maplibre/maplibre-gl-native/blob/ca5810f61fde26d0fbccbe4d48d4c771d02e460f/platform/ios/scripts/generate-shaders.js#L135-L137

@wipfli
Copy link
Contributor

wipfli commented Jun 23, 2022

I am installing android studio to see if this pull request changes something on android. Let's see if I can make an app... Haha the last time I used this was in 2015...

@wipfli
Copy link
Contributor

wipfli commented Jun 23, 2022

OK not getting very far with the android build... I cloned the repo recursively, went to the platform/android folder, and run:

➜  android git:(main) make android-configuration
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
cat gradle/configuration.gradle
ext {
    node = '/home/wipfli/.nvm/versions/node/v16.13.2/bin/node'
    npm = '/home/wipfli/.nvm/versions/node/v16.13.2/bin/npm'
    ccache = '/usr/bin/ccache'
}%                                                                                                
 ➜  android git:(main) make android-check        
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
sed: can't read tests/skipped.txt: No such file or directory
./gradlew --parallel --max-workers=8 -Pmapbox.buildtype=debug -Pmapbox.stl=c++_static -Dorg.gradle.internal.http.socketTimeout=360000 -Dorg.gradle.internal.http.connectionTimeout=360000 -Pmapbox.abis=none ktlint

> Configure project :MapboxGLAndroidSDK
WARNING: The following project options are deprecated and have been removed: 
android.enableUnitTestBinaryResources
The raw resource for unit test functionality is removed.



FAILURE: Build failed with an exception.

* Where:
Build file '/home/wipfli/maps/maplibre-gl-native/platform/android/MapboxGLAndroidSDK/build.gradle' line: 102

* What went wrong:
A problem occurred evaluating project ':MapboxGLAndroidSDK'.
> Could not open dsl remapped class cache for cf3knui2j1q81zfz22itmf0rd (/home/wipfli/.gradle/caches/6.4.1/scripts-remapped/gradle_javadoc_a4pw0ju1gppnt8pjfbirkcx7s/cf3knui2j1q81zfz22itmf0rd/dsld52bdc530b3c89c92cc1916d3edc5e92).
   > Could not open dsl generic class cache for script '/home/wipfli/maps/maplibre-gl-native/platform/android/gradle/gradle-javadoc.gradle' (/home/wipfli/.gradle/caches/6.4.1/scripts/cf3knui2j1q81zfz22itmf0rd/dsl/dsld52bdc530b3c89c92cc1916d3edc5e92).
      > BUG! exception in phase 'semantic analysis' in source unit '_BuildScript_' Unsupported class file major version 61

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.4.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 568ms
make: *** [Makefile:279: android-ktlint] Error 1

@wipfli
Copy link
Contributor

wipfli commented Jun 23, 2022

Am I supposed to compile the android stuff in the android docker container? I see that the android-ci.yml action does this...

https://github.com/maplibre/maplibre-gl-native/blob/ca5810f61fde26d0fbccbe4d48d4c771d02e460f/.github/workflows/android-ci.yml#L41-L45

@wipfli
Copy link
Contributor

wipfli commented Jul 4, 2022

44 render tests fail with this pull request (11 on main):

* failed render-tests/line-gradient/gradient
* failed render-tests/line-gradient/translucent
* failed render-tests/line-gradient/gradient-tile-boundaries
* failed render-tests/symbol-placement/line-overscaled
* failed render-tests/symbol-placement/line-center
* failed render-tests/symbol-placement/line-center-tile-map-mode
* failed render-tests/symbol-visibility/visible
* failed render-tests/runtime-styling/image-update-pattern
* failed render-tests/runtime-styling/image-add-pattern
* failed render-tests/regressions/mapbox-gl-js#5978
* failed render-tests/regressions/mapbox-gl-js#3107
* failed render-tests/regressions/mapbox-gl-native#9976
* failed render-tests/regressions/mapbox-gl-js#8273
* failed render-tests/regressions/mapbox-gl-js#5911a
* failed render-tests/regressions/mapbox-gl-js#2533
* failed render-tests/map-mode/tile-avoid-edges
* failed render-tests/text-keep-upright/line-placement-true-pitched
* failed render-tests/text-variable-anchor/rotated-offset
* failed render-tests/text-variable-anchor/all-anchors-tile-map-mode
* failed render-tests/icon-text-fit/enlargen-both
* failed render-tests/debug/collision-lines
* failed render-tests/debug/collision-lines-overscaled
* failed render-tests/debug/collision-icon-text-line-translate
* failed render-tests/debug/collision-lines-pitched
* failed render-tests/line-pattern/pitch
* failed render-tests/line-pattern/step-curve
* failed render-tests/line-pattern/zoom-expression
* failed render-tests/line-pattern/property-function
* failed render-tests/line-pattern/literal
* failed render-tests/line-pattern/overscaled
* failed render-tests/line-pattern/@2x
* failed render-tests/fill-opacity/property-function-pattern
* failed render-tests/fill-pattern/case-data-expression
* failed render-tests/fill-pattern/wrapping-with-interpolation
* failed render-tests/fill-pattern/uneven-pattern
* failed render-tests/fill-pattern/@2x
* failed render-tests/within/filter-with-inlined-geojson
* failed query-tests/regressions/mapbox-gl-js#8999-2
* failed query-tests/regressions/mapbox-gl-js#10074
* failed query-tests/circle-radius/tile-boundary
* failed query-tests/fill-features-in/default
* failed query-tests/invisible-features/zero-opacity
* failed query-tests/edge-cases/box-cutting-antimeridian-z1
* failed query-tests/edge-cases/box-cutting-antimeridian-z0
* failed query-tests/edge-cases/null-island

@wipfli
Copy link
Contributor

wipfli commented Jul 4, 2022

You can run the render test on ubuntu with something like

git clone --recurse-submodules -j8 https://github.com/maplibre/maplibre-gl-native.git
cd maplibre-gl-native

# build stuff
cmake . -B build -G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=gcc-10 -DCMAKE_CXX_COMPILER=g++-10
cmake --build build -j $(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null)

# run render test
./build/mbgl-render-test-runner --manifestPath metrics/linux-clang8-release-style.json &> render-test.log

# remove color stuff from log file
sed -i 's/\x1b\[[0-9;]*m//g' render-test.log 

@wipfli wipfli mentioned this pull request Jul 4, 2022
@wipfli
Copy link
Contributor

wipfli commented Jul 4, 2022

Here are the full render and query test results (85 MB website): https://wipfli.github.io/debug-maplibre-gl-native/linux-clang8-release-style.html

@wipfli
Copy link
Contributor

wipfli commented Jul 4, 2022

Lot's of interesting problems...

@wipfli
Copy link
Contributor

wipfli commented Jul 5, 2022

Hm, after merging main the render tests fail with something like this:

./build/mbgl-render-test-runner --manifestPath metrics/linux-clang8-release-style.json
...
* passed render-tests/text-arabic/multi-paragraph
* passed render-tests/symbol-visibility/none
* passed render-tests/symbol-visibility/visible
* passed render-tests/runtime-styling/layout-property-zoom-and-property-expression-to-zoom-expression
* passed render-tests/runtime-styling/layout-property-override-paint-property-expression
mbgl-render-test-runner: ../src/mbgl/gl/uniform.cpp:150: bool mbgl::gl::verifyUniform(const mbgl::gl::ActiveUniform&) [with T = std::array<float, 4>]: Assertion `uniform.size == 1 && uniform.type == UniformDataType::FloatVec4' failed.
[1]    66079 abort (core dumped)  ./build/mbgl-render-test-runner --manifestPath 

@wipfli
Copy link
Contributor

wipfli commented Jul 5, 2022

No idea why this happens. I will just close this pull request and re-generate the shaders and style code from main by running the update-gl-js.yml action...

@wipfli wipfli closed this Jul 5, 2022
@wipfli wipfli deleted the update-gl-js-to-49bc4ca45 branch July 5, 2022 17:27
@wipfli
Copy link
Contributor

wipfli commented Jul 5, 2022

#359 is the follow-up pull request

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.

3 participants