Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

GL extension use safety mechanism #2527

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Conversation

incanus
Copy link
Contributor

@incanus incanus commented Oct 5, 2015

@adam-mapbox I am seeing this work fine on iOS and solve the problem described in mapbox/mapbox-gl-style-spec#365 and seemingly captured in #774 a long time ago when background-pattern was called background-image.

However, the addition of glBindVertexArrayOES() doesn't work on OS X, and may cause problems on other platforms.

../gl-native/src/mbgl/renderer/painter.cpp:422:9: Use of undeclared identifier 'glBindVertexArrayOES'; did you mean 'glBindVertexArrayAPPLE'?

Take note of vao.cpp which references this function. I think we need a way to make this work generically across the platforms.

/cc @kkaefer

@incanus
Copy link
Contributor Author

incanus commented Oct 5, 2015

Per voice, assuming the Android combine fix in 68149f0 works, let's split this into two atomic changes:

  1. The one-line fix in ec87df4#diff-463080b8a13421555827d7d0edd42421R422 which fixes the root background-pattern problem.
  2. The larger change which introduces a system for trapping our string-based calls to GL extensions and warns about a) general classes of problems and b) the specific VAO binding problem captured in the source issue.

@incanus
Copy link
Contributor Author

incanus commented Oct 5, 2015

The one-line fix

👉 #2531

Waiting for CI / interactively rebasing this PR here now to address the second item.

@incanus incanus changed the title [wip] fix background pattern GL extension use safety mechanism Oct 5, 2015
@incanus incanus added build and removed bug labels Oct 5, 2015
@incanus
Copy link
Contributor Author

incanus commented Oct 6, 2015

This looks good now. @adam-mapbox is going to rebase this into a single commit and force-push for final review. Can you have a look @kkaefer? Seems sound in my mind, but a little over my head OpenGL-wise.

@incanus incanus removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 6, 2015
@ljbade
Copy link
Contributor

ljbade commented Oct 6, 2015

@incanus @kkaefer i thought we had developed a robust extension system? In the gl:: namespace that will load the correct OES or APPLE extension for VAOs?

There is even boolean variable you can use with if statements.

@ljbade
Copy link
Contributor

ljbade commented Oct 6, 2015

Oh I see now this PR lets us pick up typos for GL extensions.

A long time ago I noted we don't carefully check for unsupported extensions but that got cleaned up.

I real gotcha though is extensions that don't add new GL functions but just expand the valid values for existing functions.

@fezzik21
Copy link

fezzik21 commented Oct 6, 2015

This is a bit more than that. The point of this is really to catch logic errors that involve the use of extension functions. In this case, a particular negative pattern of buffer set up, but more generally to track issues involving patterns of calls.

Sent from my iPhone

On Oct 5, 2015, at 6:33 PM, Leith Bade notifications@github.com wrote:

Oh I see now this PR lets us pick up typos for GL extensions.

A long time ago I noted we don't carefully check for unsupported extensions but that got cleaned up.

I real gotcha though is extensions that don't add new GL functions but just expand the valid values for existing functions.


Reply to this email directly or view it on GitHub.

@kkaefer
Copy link
Contributor

kkaefer commented Oct 6, 2015

@adam-mapbox overall changeset looks good. Can you please rebase on master and squash commits into functional units? I.e. every commit must work on its own. That could also mean to squash all commits into one, if there is no intermediate step that keeps the build functional.

@incanus
Copy link
Contributor Author

incanus commented Oct 6, 2015

I think one commit works fine here.

@adam-mapbox adam-mapbox force-pushed the adam_fix_background_pattern branch 2 times, most recently from 69d1dc5 to 3c66428 Compare October 6, 2015 20:11
@adam-mapbox adam-mapbox merged commit 1179096 into master Oct 6, 2015
@adam-mapbox adam-mapbox deleted the adam_fix_background_pattern branch October 6, 2015 21:01
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants