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

Implement OES_element_index_uint support #121

Merged
merged 3 commits into from
Oct 18, 2017

Conversation

dhritzkiv
Copy link
Member

@dhritzkiv dhritzkiv commented Oct 13, 2017

I went down a rabbit of trying to debugging why some of my glTF 2 models weren't rendering in headless mode. Turned out they were using unsigned ints for the indices, which requires the OES_element_index_uint extension. While, headless-gl doesn't currently support it, I noticed that ANGLE does.

The way I added support:

  • Get the list of supported extensions from ANGLE via getSupportedExtensions (this seems to be legit, and not contain hardcoded values, as testing on different machines reported different supported extensions)
  • Check to see if GL_OES_element_index_uint is in that list
  • If it is, return a non-falsy value (that's all that seems to be required). As a bonus, the return value is an instance from an empty constructor called OESElementIndexUint, as that's what browsers seem to report.
  • Add handling of the data to the drawElements method (if the extension is enabled and supported)

Not sure if this is the right way to add support, but it worked for me, and the conformance tests for OES_element_index_uint pass.

If this is the right way to do it, perhaps this could be done for #72, as well as other extensions?

Closes #81

@dhritzkiv
Copy link
Member Author

Hmm. For some reason, the last two commits seem to be failing in CI despite their changes not affecting the tests that are failing. Tests pass locally, otherwise.

@mourner
Copy link
Collaborator

mourner commented Oct 17, 2017

Hi Daniel, thanks for the contribution! To be honest, I took over the maintenance of this module to push out some trivial updates (after the original author moved on), but I don't understand much of the underlying source code. On the surface the PR looks good though. Let's try to figure out what's going on on the CI, and merge on green.

@mourner
Copy link
Collaborator

mourner commented Oct 18, 2017

@dhritzkiv hey, I just invited you as a collaborator on this repo. Hopefully we can revive the maintenance of this project — this is an extremely useful repo that severely lacks core contributors.

@dhritzkiv
Copy link
Member Author

@mourner Great!

What actions should I take regarding the PR? I can try poking around the CI configs to see if I can fix the tests there.

@dhritzkiv
Copy link
Member Author

So I took a crack at trying to get the tests passing consistently on CI. No luck. They sometimes pass and sometimes fail. There's no rhyme or reason, and sometimes pushing the same commit again doesn't result in the same result. Some things I tried:

  • using latest node and npm
  • turning faucet off
  • switching faucet for tap-spec

@mourner
Copy link
Collaborator

mourner commented Oct 18, 2017

@dhritzkiv oh, sorry! I just realized that I had the same issue. Let's merge and try to figure out CI flakiness later.

@mourner mourner merged commit 99efeb3 into stackgl:master Oct 18, 2017
@dhritzkiv
Copy link
Member Author

Cool. Perhaps in the future we should ignore appveyor, and instead focus on the Travis builds which are running, but not connected to Github (#100)

@mourner
Copy link
Collaborator

mourner commented Oct 18, 2017

@dhritzkiv made you an admin, you can now edit web hooks.

@dhritzkiv
Copy link
Member Author

Thanks again! Unfortunately, it seems the repo owner is required to make changes to the connection between travis and this repo on travis

@dhritzkiv
Copy link
Member Author

To clarify: it seems that travis is enabled for pushes, but not for PRs.

@mourner
Copy link
Collaborator

mourner commented Oct 19, 2017

Maybe let's try changing username / token on the Travis GH hook settings page? https://github.com/stackgl/headless-gl/settings/hooks/7907966

@dhritzkiv
Copy link
Member Author

I'm trying to add stackgl as a repo to my Travis account, but I need to be granted access to do so by stackgl:

screen shot 2017-11-04 at 12 16 38

Otherwise, I fear this repo won't appear under my travis account, so changing the username/token will be moot.

@mourner
Copy link
Collaborator

mourner commented Nov 10, 2017

I'm not a member of stackgl so can't do that :( @mikolalysenko have a second?

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.

2 participants