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

Support base index for draw instancing in OpenGL #7016

Merged
merged 7 commits into from
Feb 11, 2020
Merged

Support base index for draw instancing in OpenGL #7016

merged 7 commits into from
Feb 11, 2020

Conversation

YTN0
Copy link
Contributor

@YTN0 YTN0 commented Feb 10, 2020

Per #6732 ... This change exposes the option to set the instance base index for instancing. This works perfectly on DirectX, and somewhat works on OpenGL. On OpenGL, this is still impacted by the instancing bug #6293.

Note: For OpenGL, if not setting the base instance index (which is the default behavior), it will continue using the original OpenGL API. If setting the base, this will use the OpenGL API glDrawElementsInstancedBaseInstance which is only available in OpenGL 4.2 and higher.

Also added a capabilities check to GraphicsCapabilities.cs. If the glDrawElementsInstancedBaseInstance is not available (i.e. < OpenGL 4.2), this defaults to the default behavior and calls the original method without attempting to set the base index.

FYI @Jjagg Let me know if this looks good.

@YTN0 YTN0 changed the title Fix 6732 Fix #6732 - Support base index for instancing Feb 10, 2020
@YTN0 YTN0 requested a review from Jjagg February 10, 2020 04:53
@YTN0
Copy link
Contributor Author

YTN0 commented Feb 10, 2020

@nkast Modified to retain original APIs and added overloaded versions.

@nkast
Copy link
Contributor

nkast commented Feb 10, 2020

Thanks @YTN0

Here is a recent issue that describes in more details the problems with default parameters.
#6682

@Jjagg
Copy link
Contributor

Jjagg commented Feb 10, 2020

We're getting there @YTN0! Great work so far!

2 more things

  • I don't think we should add the obsolete overload
  • when a base index is passed and it is not supported, I think we should throw an exception. I don't know what type of exception we usually throw in cases like this, but I'll check when I get to my laptop!

@YTN0
Copy link
Contributor Author

YTN0 commented Feb 11, 2020

@Jjagg Made the changes you suggested. Removed the obsolete override, and also changed the code to throw a PlatformNotSupportedException if trying to use base index instancing when < OpenGL 4.2 / the API is not supported.

@Jjagg
Copy link
Contributor

Jjagg commented Feb 11, 2020

Awesome, thanks @YTN0! Gonna merge this one :)

I'm thinking it would be good if we had a single page in the docs that listed all graphics capabilities and what platform native caps they match with for the DX and GL backend. And we should also add the graphics capabilities requirements to the xml docs for the specific methods IMO. But for now this is great!

@Jjagg Jjagg merged commit 2acaa3e into MonoGame:develop Feb 11, 2020
@YTN0 YTN0 deleted the fix_6732 branch February 12, 2020 03:41
@nkast
Copy link
Contributor

nkast commented Jun 1, 2020

The pattern we use throughout the API is that base/index go first before count. I think we should change those new methods to follow the same pattern to be consistent before the final 3.8 release.

@harry-cpp harry-cpp added this to the 3.8 Release milestone Jun 14, 2020
@tomspilman tomspilman changed the title Fix #6732 - Support base index for instancing Support base index for instancing Aug 1, 2020
@tomspilman tomspilman changed the title Support base index for instancing Support base index for draw instancing Aug 1, 2020
@tomspilman tomspilman changed the title Support base index for draw instancing Support base index for draw instancing in OpenGL Aug 1, 2020
kimimaru4000 pushed a commit to kimimaru4000/MonoGame that referenced this pull request Sep 25, 2020
* Fix for 6732

* Making baseInstance optional in one of the modified methods

* Updating default parameter

* Updating GraphicsDevice.Web with default parameter for base index instancing

* Maintained original APIs and added overloaded versions

* Removing obsolete instancing method override and throwing exception when base index instancing is used but < OpenGL 4.2

* Changing exception verbiage slightly to match API and parameter name
kimimaru4000 pushed a commit to kimimaru4000/MonoGame that referenced this pull request Sep 25, 2020
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.

4 participants