igl | metal | MTLRenderCommandEncoder::draw() fix the missed instanceCount on versions below iOS 16.

[vinsentli]

No description provided.

[facebook-github-bot]

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[EricGriffith]

I'm a little worried about this one. I think we chose iOS 16 because it's a guarantee that all devices on that iOS version would actually support it. The proper fix for detecting whether it is actually supported or not is to handle this via DeviceFeatures.

[vinsentli]

Strictly speaking, there are also some issues with the else branch. The instanceCount parameter is lost, although Metal actually supports it.

[corporateshark]

These new changes do no address what @EricGriffith mentioned about the version 9 and igl::DeviceFeatures.

[vinsentli]

@corporateshark
From Apple's documentation and SDK, the information I obtained is:

MTLRenderCommandEncoder::draw:instancecount: is available from iOS 8.0.
MTLRenderCommandEncoder::draw:instancecount:baseInstance: is available from iOS 9.0.

@EricGriffith Have you encountered any devices running between iOS 9 and iOS 16 (exclusive) that don't support baseInstance?

https://developer.apple.com/documentation/metal/mtlrendercommandencoder/drawindexedprimitives(type:indexcount:indextype:indexbuffer:indexbufferoffset:instancecount:basevertex:baseinstance:)?language=objc

[EricGriffith]

@vinsentli I don't recall the specifics of why we set it up this way. It's been over a year. However, the consequences for us getting this wrong are pretty severe. We need to make sure we're properly checking MTLDevice features.

[vinsentli]

@vinsentli I don't recall the specifics of why we set it up this way. It's been over a year. However, the consequences for us getting this wrong are pretty severe. We need to make sure we're properly checking MTLDevice features.

@EricGriffith

We tested on iOS 12.5 devices and found that this API is not supported, which causes crashes. Are you encountering the same issue? As a result, I've reverted the changes related to the @available version and only kept the fix for the missing instanceCount issue on versions below iOS 16.

[facebook-github-bot]

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@corporateshark merged this pull request in dded52c.