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

SDL_RenderGeometry [patch] #772

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 4 comments
Closed

SDL_RenderGeometry [patch] #772

SDLBugzilla opened this issue Feb 10, 2021 · 4 comments
Labels
enhancement New feature or request waiting Waiting on user response

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 10, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: 2.0.0
Reported for operating system, platform: All, All

Comments on the original bug report:

On 2013-02-26 08:10:39 +0000, Gabriel Jacobo wrote:

The attached patch implements the following functions on OpenGL/ES/ES2.

extern DECLSPEC int SDL_RenderGeometry(SDL_Renderer * renderer, SDL_Texture *texture, SDL_Vertex vertices, int num_vertices, int indices, int num_indices, const SDL_Vector2f *translation);
extern DECLSPEC int SDL_EnableScissor(SDL_Renderer * renderer);
extern DECLSPEC int SDL_DisableScissor(SDL_Renderer * renderer);
extern DECLSPEC int SDL_ScissorRegion(SDL_Renderer * renderer, const SDL_Rect *region);

I've used most of this to integrate libRocket in my engine, and given that now I want to integrate the Spine runtime and they use a similar mechanism, I figured abstracting the functionality inside SDL would be beneficial to others.

SDL_RenderGeometry can be also thought of as a super set of RenderCopy/Ex and the drawing functions too, given it can work with and without a texture (if the texture is null, vertex colored primitives are rendered).

On 2013-02-26 09:42:05 +0000, Gabriel Jacobo wrote:

Created attachment 1054
RenderGeometry v1

On 2013-02-26 10:26:47 +0000, Sam Lantinga wrote:

Do you have a Direct3D and software implementation? :)

On 2013-02-26 10:44:00 +0000, Gabriel Jacobo wrote:

If you give me a pass on the software renderer, I'll work on a D3D implementation :)

Seriously though, there's no way a pure software implementation of this has any chance of being actually usable, so it would be a lot of work for what's basically a scientific experiment.

On 2013-02-26 10:46:04 +0000, Sam Lantinga wrote:

Yeah, in that case I would recommend just using OpenGL directly. Is there any reason you're not doing that in your use case?

On 2013-02-26 11:08:29 +0000, Gabriel Jacobo wrote:

I'm rehashing the RenderCopyEx arguments a bit here...

  • For a good number of SDL users, implementing this functionality on their own is way above the difficulty level of just using the library.

  • SDL has a ton of useful GL goodies in it (dynamic library loading, shaders and shader management, texture loading, render targets, etc), but it keeps them close to the chest, so they are not usable externally and have to be literally copy pasted, duplicating code.

  • Using OpenGL functions "externally" while also using SDL_Render* functions is cumbersome as SDL also keeps its internal state hidden, hence you have to be extra careful to leave everything as it was when you give control back to SDL, and resort to ugly tricks (such as drawing a fake point outside the screen to force SDL to a known state) on top of that.

  • The use case for SDL_RenderGeometry is common enough, as I mentioned it can be thought of a generalization of the SDL_RenderCopy/Ex and drawing functions.

  • SFML has it ;)

  • There seems to be others interested in it.

On 2013-02-27 12:56:58 +0000, Philipp Wiesemann wrote:

The documentation of SDL_RenderGeometry() added to SDL_render.h by the patch says "The SDL texture to unbind". I think it should be "bind".

There is a // line comment in SDL_render_gles2.c which may offend some C compilers.

Also in SDL_render_gles2.c maybe "GLushort" can be used instead of "unsigned short" because this is the type OpenGL expects (although they should be the same and I do not know if it matters somewhere).

On 2013-03-05 08:07:36 +0000, Gabriel Jacobo wrote:

*** Bug 1716 has been marked as a duplicate of this bug. ***

On 2013-07-06 10:31:20 +0000, Philipp Wiesemann wrote:

One call to glVertexAttribPointer() is not through function pointer (like data->glVertexAttribPointer).

On 2013-07-06 10:47:36 +0000, Gabriel Jacobo wrote:

That's right, I had fixed this here: https://bitbucket.org/gabomdq/sdl-1.3-experimental/commits/ee254b65616e72cdf0318a988faa4da0c7a3de84 (the attached patch is probably outdated, so the reference for this should be my fork on Bitbucket).

On 2013-10-15 13:04:44 +0000, Gabriel Jacobo wrote:

Created attachment 1368
RenderGeometry v2

Given renewed interest in this, I'm attaching an updated patch. Note that this patch contains SDL_EnableScissor / SDL_DisableScissor / SDL_ScissorRegion which are now (probably) made redundant by SDL_RenderSetClipRect.

On 2014-02-25 19:46:59 +0000, Gabriel Jacobo wrote:

*** Bug 2420 has been marked as a duplicate of this bug. ***

On 2017-01-27 13:47:59 +0000, Tom Seddon wrote:

I needed this for my own project, which I was intending to use dear imgui with, so I applied the patch to a fairly recent SDL head and added D3D9 and D3D11 versions: https://github.com/tom-seddon/SDL/tree/_RenderGeometry

Software rendering is still missing... I don't think I have a need for it.

This now plays nicely with dear imgui: https://github.com/tom-seddon/SDL/wiki/dear-imgui

Hasn't been very exhaustively tested, but it seems to work, so what could possibly go wrong? But assuming I don't encounter some surprise problem with dear imgui that makes it unsuitable, I'll be continuing to use it and will fix any issues I discover. I should think I will also occasionally keep the branch up to date wrt SDL head.

I have a couple of possible suggestions for improvements to the API but I'm going to sleep on them for a couple of days.

--- what's untested ---

  • YUV textures
  • OpenGLES (but the original patch applied cleanly, so fingers crossed...)
  • anything apart from Windows 10 or OS X El Capitan (I tested the GLES2 version on Windows)

--- patch changes ---

Compared to the original patch, I tweaked a few things:

  • select shader before using matrix stack (don't know if this is my PC's GPU, or a standard OpenGL thing when mixing matrix stack and shaders)
  • select renderer blend mode when rendering if there's no texture (this seems like the sensible thing)
  • remove scissor stuff, since the renderer clip rect covers it
  • use texture's native version, if it has one
  • unbind textures after use (necessary when running with shaders disabled)

--- d3d9/d3d11 notes ---

The D3D9/D3D11 code is basically just the obvious thing, but I did have to make two changes:

  • D3D9 changed to use a vertex declaration rather than an FVF (since FVF can't support RGBA vertex colours or float2 vertex coordinates)
  • D3D11 now uses the SDL_Vertex format internally, so SDL_Color for vertex colours rather than float4 (which is probably an improvement anyway as it saves on some conversion)

On 2017-01-27 14:01:35 +0000, Gabriel Jacobo wrote:

Cool, thanks! Now all we need is to convince Sam to let us merge this without the software backend :)

On 2017-01-27 15:14:20 +0000, Alex Szpakowski wrote:

If SDL_RenderGeometry is not supported with software rendering then a new SDL_RendererFlag enum value should probably be added, so people can query for support.

The *_AUTOSCALE shader variants are broken because they use the GLSL 1.30+ function 'textureSize' without specifying a GLSL version (therefore the driver will default to GLSL 1.10). Some drivers may accept this, however they are not compliant with the OpenGL spec if they do.

Also, the patch queries for GL_ARB_shading_language_100 as part of its logic to determine whether to use the textureSize-capable shaders, but that extension was never part of core OpenGL and drivers are free to support GL2+ with GLSL 1.10 without exporting that extension.

Also, some drivers support OpenGL 2.1 and Core Profile OpenGL 3.2, without supporting GLSL 1.30 (therefore they do not support using the modern 'textureSize' function in GLSL code that also uses deprecated functionality such as texture2D, gl_MultiTexCoord0, gl_Vertex, etc), so personally I would not recommend using textureSize at all because it's extra code ( = more fragility) in a codepath that is divergent from what the modern OpenGL API requires while still requiring the use of semi-modern OpenGL APIs.

On 2018-12-20 01:10:02 +0000, Tom Seddon wrote:

(Prompted by https://twitter.com/icculus/status/1075533701575000064)

I'm still using this. It's been very useful to me, and I've been keeping up to date with HEAD when I can be bothered. But the last merge was a bit of a pain, even taking into account the fact I just ignored Metal entirely! - and now it seems Metal is (very sensibly) going to become the default on Apple platforms. So I guess at some point in the future I'm going to have some more work ahead of me.

This certainly isn't a complaint, as my fork has just been me doing my own thing, on my own, for my own project, because it was useful to me personally. But now that my fork is starting to fall behind, it does make me wonder what its future should be. Is there any wider interest in this? Is there any interest in having it included in SDL proper, so that its future maintenance can be somebody else's responsibility? And if there is, what do I need to do to make that happen?

(One thing I'm afraid I'm not going to do myself is anything about software rendering support, as this feels like it would be a bunch of work, and it isn't useful to me. I do think it would be a good feature, maybe even better than that, and if I've done anything that would get in its way, then I made a a mistake, and I'll try to fix it! - but I'm just not interested in writing any of the code for it myself.)

Some anticipated responses:

"No": OK, and probably not unreasonable, if the majority of SDL users get by without it ;) - I will adjust my future plans accordingly.

"Yes, but only if there's software rendering support": equivalent to "no" from my perspective, so, ditto.

"Yes": I will book some time to add Metal support, get it all merged in with latest HEAD, hopefully to general approval, and the SDL maintainers can take it from there.

<>: please specify!

Thanks,

--Tom

On 2018-12-26 06:47:04 +0000, Sam Lantinga wrote:

Ryan, what do you think about this, now that we've refactored the renderer quite a bit?

On 2019-01-02 08:08:36 +0000, Ryan C. Gordon wrote:

(In reply to Sam Lantinga from comment # 16)

Ryan, what do you think about this, now that we've refactored the renderer
quite a bit?

This is definitely something I want to revisit in early 2019.

--ryan.

On 2020-05-03 10:56:06 +0000, Albert Vaca Cintora wrote:

Hi Ryan, any chance this can be merged? If not, what is blocking it?

This is a really cool feature and I feel it was almost ready to merge... it would be a pity if it doesn't make it in!

On 2020-11-05 00:42:30 +0000, wrote:

Created attachment 4505
GenericRenderGeometryRasterizer

In the interest of getting this going again, I've attached an implementation of a software triangle rasterizer that I wrote. It's accurate, not super slow, and works with accelerated backends as well (using SDL_RenderCopy and color/alpha mod one pixel at a time).

I've been testing it with ImGui and it's accurate on everything I've seen so far — it can handle normal rectangles, lines, copied textures, flipped textures, anti-aliased windows, round corners, etc.

There's certainly more room for optimization, especially around triangles which don't cover much of their bounding box, but for a normal-ish number of triangles works fine.

On 2020-11-05 18:56:22 +0000, Sam Lantinga wrote:

Thanks! We'll look at this for SDL 2.0.15.

On 2020-12-23 21:56:33 +0000, Sam Lantinga wrote:

@tom Seddon, I'd like to get this in for the next release.

I notice you're still keeping your tree up to date. Can you attach a unified patch to this bug with your changes and add a comment with the current status and any caveats?

Thanks!

@SDLBugzilla SDLBugzilla added enhancement New feature or request waiting Waiting on user response labels Feb 10, 2021
@tom-seddon
Copy link

tom-seddon commented Feb 13, 2021

Really sorry about this, but I'm actually now planning to abandon my fork, and the RenderGeometry patch. I did get started on updating it to latest SDL over Xmas, but changes to SDL mean it'd effectively be a rewrite, so it can cooperate nicely with the batching mechanism.

I got GL working, but GLES2 was starting to look like it'd be a bit of work, and would have required some modifications to some of the existing SDL code too. Didn't get as far as looking at D3D9/D3D11/Metal.

I just don't think I'll have the time for this sort of project in 2021, or, if I'm honest, the motivation to make the time. It's a lot like the sort of thing I do at work; feels like I'd be spending far too much time in the same room, at the same desk, doing the same thing. Might be different if I was still working at the office.

I will be continuing to use SDL, and I'm currently intending to switch to bgfx for the rendering.

I may revisit my plans if I encounter any difficulties.

Thanks,

--Tom

@1bsyl
Copy link
Contributor

1bsyl commented Mar 13, 2021

@ligfx
About the GenericRenderGeometryRasterizer, which I tried to experiment with:

The pros is that is doesn't need any back-end modification
But, its cons:

  • This is going to be very slow when usual back-end can handle triangle.
  • There is sometime a RenderCopy / per pixel, which seems crazy!, but yes, this is a good trick to be generic, and I believe this is the choice.

Just the issues, I noticed :

  • clipping could be improved: you reject all triangles, where they could be intersected with the clip rect.
  • bias1/2/3 are wrongs ! there is typo (in the 'else' part, it should be about 'y' coordinates) because it doesn't match correctly the top left rule of opengl renderers.

@ligfx
Copy link
Contributor

ligfx commented Mar 13, 2021

The pros is that is doesn't need any back-end modification

Yes, that was my overriding design constraint (as I'm using the code on SDL right now, and didn't want to have to use the software renderer). Some speed improvements were avoided in favor of having a more simple renderer that works on any backend.

bias1/2/3 are wrongs ! there is typo (in the 'else' part, it should be about 'y' coordinates) because it doesn't match correctly the top left rule of opengl renderers.

Hmm, you might be right. I'd have to go back and test it out to make sure it matches OpenGL exactly. With ImGUI, at least, the results look accurate to my naked eye.

@icculus
Copy link
Collaborator

icculus commented Aug 9, 2021

Closing this because we're likely merging @1bsyl's work in #4195 for 2.0.18.

@icculus icculus closed this as completed Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting Waiting on user response
Projects
None yet
Development

No branches or pull requests

5 participants