TINYGL: Expose TinyGL API for scissor test#6412
TINYGL: Expose TinyGL API for scissor test#6412cmd05 wants to merge 4 commits intoscummvm:masterfrom
Conversation
cmd05
force-pushed
the
tinygl-scissor-test
branch
from
433ec59 to
08ef431
Compare
lephilousophe
left a comment
lephilousophe
left a comment
There was a problem hiding this comment.
Thank you for the PR.
First, I would advise to take a quick read over the OpenGL specification (no need to read it fully).
The OpenGL 1.3 specification targets what TinyGL implements and is simpler to read.
I advise you to search for the "scissor" word, and read the start of the chapter 2 (OpenGL basics) and the start of the chapter 4 (where the scissor happens).
Then, I would say that there have been some misnaming in the past which may cause troubles to understand the code and which we may take the opportunity to fix. That's why I propose to rename the existing scissorRect to clippingRect (or another fitting name) while adding the new scissorRect.
Both should stay independent and tested. This will avoid you modifying the existing clipping rectangle.
This will then need the additional checks implemented.
That means that everywhere the (currently named) variable _enableScissor is checked, the new _enableScissorTest variable should be tested as well and the scissorPixel function should be extended to really check for the GL scissor in addition to the clipping rectangle.
In addition, the clipBlitImage will need to be adapted.
Another step could be done to clean things further up by removing the (currently named) _scissorRect variable in GLContext as this should have never been here.
The blitting code should use the framebuffer clipping rectangle instead.
Finally it may be interesting (not sure about this) to update the various computeDirtyRegion to take the scissor box into account.
| // nothing to do | ||
| } | ||
|
|
||
| void tglScissor(TGLint x, TGLint y, TGLsizei width, TGLsizei height) { |
There was a problem hiding this comment.
The signature of tglScissor should be void tglScissor(TGLint left, TGLint bottom, TGLsizei width, TGLsizei height); as per the OpenGL specification.
| break; | ||
| case TGL_SCISSOR_BOX: | ||
| // fall through | ||
| error("gl_get_pname: TGL_SCISSOR_BOX option not implemented"); |
There was a problem hiding this comment.
This should be easy to implement: see TGL_VIEWPORT case for inspiration.
| _profilingEnabled = false; | ||
|
|
||
| // scissor test | ||
| scissor_test_enabled = false; |
There was a problem hiding this comment.
You can init the Scissor box here.
As per the spec:
In the initial state lef t = bottom = 0; width and height are determined by the size of the GL window.
That means covering the whole window: you can reuse the renderRect for this.
|
|
||
| void GLContext::glopScissor(GLParam *p) { | ||
| // top left corner | ||
| _scissorTestRect.top = p[2].i; |
There was a problem hiding this comment.
This is not correct: scissor is defined (like almost all OpenGL calls) starting from the bottom left of the screen.
| _scissorTestRect.left = p[1].i; | ||
|
|
||
| // bottom right corner | ||
| _scissorTestRect.bottom = p[4].i; |
There was a problem hiding this comment.
This is not correct either as params 3 and 4 are width and height.
| _polygonStippleEnabled = enable; | ||
| } | ||
|
|
||
| void enableScissorTest(bool enable) { |
There was a problem hiding this comment.
Please move this out of the middle of the Stipple functions.
Before Alpha test seems a better place as the test must happen before the alpha test (per the spec).
| FrameBuffer(int width, int height, const Graphics::PixelFormat &format, bool enableStencilBuffer); | ||
| ~FrameBuffer(); | ||
|
|
||
| bool getScissorTestEnabled() { |
There was a problem hiding this comment.
I don't think you need them.
| } | ||
|
|
||
| void BlittingDrawCall::execute(const Common::Rect &clippingRectangle, bool restoreState) const { | ||
| // set scissor rectangle here |
|
|
||
| void ClearBufferDrawCall::execute(const Common::Rect &clippingRectangle, bool restoreState) const { | ||
| TinyGL::GLContext *c = gl_get_context(); | ||
|
|
There was a problem hiding this comment.
Same as above: don't mess with this clipping rectangle and keep things separate.
|
@cmd05 Any updates? |
|
Closing in favor of #6575 |
6 participants
This PR exposes TinyGL's API for
tglScissor()andtglEnable/Disable(TGL_SCISSOR_TEST). I have currently tested the commit on freescape's total eclipse demo. The commit works for rasterization, however scissor test for clearing (screen clear color) and blitting needs to be fixed. I have added comments to the sections of the code where clearing and blitting occur. The relevant caller to these functions is here. This will require some inspection of the clear and blitting code.Example rasterized output when using a half viewport sized scissor:
Output of the Freescape Total Eclipse demo with commits applied (scissor for clear color needs to be implemented):
Expected Output (current Freescape TinyGL implementation uses a hack for clearing color by coloring a viewport sized quad):
Additionally, the implementation should decide whether to provide a default scissor rectangle in case
tglEnable(TGL_SCISSOR_TEST)is called buttglScissoris never called (need to check from OpenGL documentation). Currently, this will lead to no output drawn, sinceCommon::Rectwill have no dimensions by default.