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

Shader-based rendering (based on mirv's branch) #381

Closed
wants to merge 70 commits into from
Closed

Shader-based rendering (based on mirv's branch) #381

wants to merge 70 commits into from

Conversation

seedhartha
Copy link
Contributor

@seedhartha seedhartha commented Nov 4, 2018

This PR introduces experimental shader-based (GL3.2) renderer. It is based on mirv's branch (last commit adeab18) with the following changes:

  • All individual commits are now compilable, there is almost no overlap
  • Fixed formatting, removed commented out code and debug output
  • Removed configuration option "opengl3", GL3.2 is required for experimental rendering, otherwise GL2.1 is forced
  • Added configuration option "rendernew" to toggle experimental renderer
  • Fixed rendering of NWN portraits with both legacy and experimental renderer
  • Fixed segfault on exit in KotOR

@mirv-sillyfish
Copy link
Contributor

Is there a reason NWN shader rendering was disabled? If there's some issue, I should try to fix it. That's all I can think of to comment about really!

@seedhartha
Copy link
Contributor Author

seedhartha commented Nov 4, 2018

Not disabled really, just removed the commit that enables it by default because of some minor regressions in GUI (e.g. portraits).
As a side note, I believe that multiple render paths and shader versions introduced by GL2/3 switch are unnecessary: we do not have the manpower to support that.

@seedhartha
Copy link
Contributor Author

I'm going to replace setRendererExperimental method with a configuration option (for flexibility), dump "opengl3" option and remove support for GL2.1 shaders (for reduced complexity). Therefore there will be two rendering paths only: legacy GL2.1 (FFP + VBO, no shaders) and experimental GL3.2 (VAO + shaders). @mirv-sillyfish, any thoughts why that may be a bad idea?

@mirv-sillyfish
Copy link
Contributor

So GL3.2 (core) profile isn't supported everywhere. That's the reason for the GL2.1 code path: to allow shader introduction until GL3.2 (core) is available across all engines & render pathways. This is also the reason the GL3.2 path is enabled only via command line switch.

For the current level of development, it's not so bad to have differing pathways, and not so difficult to strip out. Once other shader get introduced (Jade Empire for example) that target one particular game, then things can be a bit more interesting.

Side note, I have gui rendering issues if I don't use the shader pathway here. Renders correct with shaders enabled (for NWN).

@seedhartha
Copy link
Contributor Author

seedhartha commented Nov 7, 2018

That's the reason for the GL2.1 code path: to allow shader introduction until GL3.2 (core) is available across all engines & render pathways.

That's a valid point. Why not introduce both GL3.2 and shaders simultaneously though (per engine)? Prior to my changes there were 4 possible render paths: GL2.1 w/o shaders, GL2.1 + shaders, GL3.2 w/o shaders (broken) and GL3.2 + shaders. It is easier to deal with now, with a single configuration option.

Side note, I have gui rendering issues if I don't use the shader pathway here. Renders correct with shaders enabled (for NWN).

Yes, portraits are not being displayed with both legacy and experimental renderer for me. Could you look into it?

@seedhartha
Copy link
Contributor Author

@DrMcCoy, could we merge this after NWN portraits get fixed and KotOR animations are reimplemented? What are your thoughts on the matter? There should be no breaking changes as long as experimental renderer is not enabled.

@mirv-sillyfish
Copy link
Contributor

Yes, portraits are not being displayed with both legacy and experimental renderer for me. Could you look into it?

I'll investigate - might take me a few days to get the time, but I will have a look.

@seedhartha seedhartha changed the title Renderer rewrite (based on mirv's branch) Shader-based rendering (based on mirv's branch) Nov 8, 2018
@seedhartha
Copy link
Contributor Author

seedhartha commented Nov 10, 2018

I'll investigate - might take me a few days to get the time, but I will have a look.

Nevermind, I got it. GL2.1 portraits weren't rendered because the rendering path used shaders and perspective projection was never set. GL3.2 portraits were broken because of a bug in MeshQuad: it allocated vertex buffer twice.

@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 10, 2018

Thank you for putting the time into this @seedhartha, and also of course thanks to @mirv-sillyfish :)

If you're both happy with this, and it doesn't break anything, I'd be happy to merge this as well.

I'd still like to go through the commits themselves as well, though, because I'm me and I'm nitpicky as hell :P

@farmboy0
Copy link
Contributor

I think it would be better not to add too many new commits to this PR. This just makes reviewing longer. If those last commits arent strictly necessary maybe move them to another PR?


// OpenGL 3.2 context not created. Spit out an error message, and try a 2.1 core context.

warning("Your graphics card hardware or driver does not support OpenGL 3.2. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic comment: strictly speaking the hardware/driver does not support OpenGL 3.2 (compat profile). Compat support is relatively recent in Mesa for example, but it's had GL3.2 core profile support for quite a while.
(--edit: I know, I did it wrong, but should still be fixed)

else {
warning("Your graphics card hardware or driver does not support OpenGL 3.2. "
"Usage of experimental renderer is not possible");
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of blindly returning false, should really switch over and try the GL2.1 profile. Or add a command line switch to use GL2.1 instead of 3.2 (which then also allows easy testing to switch profiles and render pathways).

@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 17, 2018

Okay, merged it as 5ce37f9...1d03c27, thanks! :D

Here's a few things I noticed, that are broken:

NWN:

  • Health bars don't render
  • GUI boxes around conversations don't render
  • Anchors for the creature/object tooltips are wrong (project/unproject?)
  • GUI scrollbars don't render (+ crash when clicking on them [1])

NWN2:

  • Lots of messages about "probable mesh duplication"
  • Indoor tile floors are rotated wrongly (radian/degree?) [3]
  • Outdoor terrain doesn't render

KotOR/KotOR2 (possible fixed by #382?)

  • Most GUI elements (apart from text) don't render
  • Broken textures, misaligned heads [2]

Jade:

  • Crash at the start, after the intro movies ([1] again)

Sonic:

  • Nothing renders at all

Witcher:

  • Crash during shader compilation [4]

Dragon Age: Origins/Dragon Age II:

  • Lots of messages about "probable mesh duplication"
  • (But apart from that, it all works, and Dragon Age II renders better!)

[1]

xoreos: src/graphics/aurora/texturehandle.cpp:86: Graphics::Aurora::Texture& Graphics::Aurora::TextureHandle::getTexture() const: Assertion `!_empty' failed.

[2]
20181117t205221

[3]
20181117t205628

[4]

ERROR: shader compile failure! Driver output: 0(18) : error C1503: undefined variable "_colour"

@DrMcCoy DrMcCoy closed this Nov 17, 2018
@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 17, 2018

Since the leaks also happen with the old path, I consider them to be a higher priority than fixing the rendering issues with the new path, but YMMV :P

@seedhartha seedhartha deleted the mirv branch November 18, 2018 03:08
@seedhartha
Copy link
Contributor Author

@DrMcCoy, could you provide steps to reproduce the leaks?

@DrMcCoy
Copy link
Member

DrMcCoy commented Feb 6, 2019

I think I was just entering the game proper, or in the case of NWN moving to another area.

But I don't really remember anymore, and I can't check right now. Will do so later in the evening.

@DrMcCoy
Copy link
Member

DrMcCoy commented Feb 6, 2019

Yeah, for NWN, I went into the game proper (original campaign, prelude), talked to Bim to get the door to open, and went into the next area. Then I pressed ESC to get the ingame menu, quit to main menu, and from there, quit game.

That gives me this full log: https://gist.github.com/DrMcCoy/f6fc5a5eafb7bdb0839504a13d5d414f , with leaks at the bottom there

@DrMcCoy
Copy link
Member

DrMcCoy commented Feb 6, 2019

For Dragon Age, I let the game load (into that first lobby area, or whatever that is), rotated the camera a bit, then quit using ctrl+q

Full log here: https://gist.github.com/DrMcCoy/2e61e8e9fbeb880594dcbea37a9ff73e

@DrMcCoy
Copy link
Member

DrMcCoy commented Feb 6, 2019

#418 fixes this, it seems. Too tired to give it the proper attention right now tho

@seedhartha
Copy link
Contributor Author

Yep, it does fix the leaks.

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.

None yet

5 participants