scp-fs2open / fs2open.github.com

Origin Repository for SCP FreeSpace 2 Open
https://www.hard-light.net/
Other
406 stars 162 forks source link

gr.ScreenToBlob() causes many OpenGL log errors #5436

Open MjnMixael opened 1 year ago

MjnMixael commented 1 year ago

Admittedly the only symptom of the issue that I can find is many OpenGL log entry errors. Otherwise everything seems to work just as expected. But on the off chance there's a deeper issue, I'm putting this up here.

The log messages always come in sets of 5:

OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error OpenGL Debug: Source:OpenGL Type:Error ID:1282 Severity:High Message:Error has been generated. GL error GL_INVALID_OPERATION in ReadBuffer: (ID: 3653231049) Generic error

I noticed this in SCPUI as it generates blobs for ship and weapon icons. The process is.. 1: create a texture 2: draw the object to the created texture 3: save that texture to a blob 4: unload the texture

This is done in the background and never drawn to the screen.

The relevant LUA code looks like this

--arbitrary details used specifically for icon generation
modelDetails.width = 128
modelDetails.height = 112
modelDetails.heading = 50
modelDetails.pitch = 15
modelDetails.bank = 0
modelDetails.zoom = 1.1

local tex_h = gr.createTexture(modelDetails.width, modelDetails.height)
gr.setTarget(tex_h)
gr.clearScreen(0,0,0,0)
model_h:renderTechModel(0, 0, modelDetails.width, modelDetails.height, modelDetails.heading, modelDetails.pitch, modelDetails.bank, modelDetails.zoom, false)
local icon = gr.screenToBlob() --actual icon image is saved to a global, but for the purposes of testing this is probably fine

gr.setTarget()
tex_h:unload()
MjnMixael commented 1 year ago

Documenting discussion from Discord. A good way to test this is the SCPUI mod on Knossos. It runs several ScreenToBlob() calls after game init but before pilot selection during the SCPUI splash screen. The script in question is gen_icons-sct.tbm in data/tables.

A quick debug shows that it's something on line 310 of gropengl.cpp that causes OpenGL to request a log print for an invalid operation.

MjnMixael commented 1 year ago

I poked at it just a little more. I was just curious what would happen if I commented out glReadBuffer(GL_FRONT); (line 310). And... it actually works. Which surprised me because based on the documentation that line seems necessary for the glReadPixels() method.

For full clarity, commenting out glReadBuffer(GL_FRONT); stops all the invalid operations from being logged and the gr.screenToBlob() stuff still all works as expected.

qazwsxal commented 1 year ago

As is, using gr.screenToBlob() doesn't fell particularly ergonomic here, and the name of it doesn't particularly reflect what's actually being converted to a blob. I don't know if I can solve this without breaking script compatibility. Would a method on tex_h to do the same thing without the errors be useful and make more sense? i.e:

local tex_h = gr.createTexture(modelDetails.width, modelDetails.height)
gr.setTarget(tex_h)
gr.clearScreen(0,0,0,0)
model_h:renderTechModel(0, 0, modelDetails.width, modelDetails.height, modelDetails.heading, modelDetails.pitch, modelDetails.bank, modelDetails.zoom, false)
local icon = tex_h.toBlob() --actual icon image is saved to a global, but for the purposes of testing this is probably fine

gr.setTarget()
tex_h:unload()

Plan is to make this work for any texture, not just render targets.

MjnMixael commented 1 year ago

The method doesn't matter much as long as it works. So yeah that would work as long as the current ScreenToBlob sticks around as it's used error-free in other situations.

That said, did you see the above where commenting out that one line does fix the errors while the whole thing continues to still work? I tested all my use cases with that commented out and functionally seems unhindered.

qazwsxal commented 1 year ago

I'm a bit apprehensive about removing glReadBuffer(GL_FRONT); as a fix. I think it's necessary for when you're getting pixels of what's currently being displayed on screen, hence why it's used here and in the printscreen function. That said, I'd be interested to know more details about the times it does work to try and nail down if it's an issue due to things not being fully initialised yet or something.

MjnMixael commented 1 year ago

Hmm, you know now that you say it, I think I forgot to test one instance where I do just grab the actual rendered screen rather than a target.

Either way, though, it does seem like a texture:toBlob() would be a useful addition here.

BMagnu commented 1 year ago

Would it make sense to make the glReadBuffer conditional, only actually changing to the frontbuffer if there is currently no non-screen rendertarget set?

MjnMixael commented 1 year ago

Yeah that might work, too