simias / rustation

Playstation emulator in the Rust programing language
Other
552 stars 21 forks source link

Migration to glium #15

Closed Yamakaky closed 8 years ago

Yamakaky commented 8 years ago

There is a stack overflow in a previous commit, I'm tracking it.

Yamakaky commented 8 years ago

a1a0c73 is the first commit to stack overflow in debug mode.

Yamakaky commented 8 years ago

It should be ok, the screen is similar to before the migration.

Yamakaky commented 8 years ago

I need to refactor the commits ^^

simias commented 8 years ago

Very impressive! I'll review all that as soon as possible.

I've just pushed an implementation of the PSX GPU drawing area using the OpenGL scissor box so that will conflict with your changes, sorry. Hopefully it shouldn't be too difficult to integrate that change...

I haven't looked at the code yet but I've noticed that with your changes all the polygons appear brighter:

rustation-glium

Compare with the current raw OpenGL version:

rustation-opengl

In my "hack" branch I've made some dirty changes to boot up the japanese version of Crash Bandicoot, backporting it on your branch (along with the XY_FIFO fix) seem to work well, although it's still too bright:

crash-glium

My version (with the scissor box that prevents the polygons from leaking everywhere...):

crash-opengl

Yamakaky commented 8 years ago

Looks like it's missing gamma correction ^^

Yamakaky commented 8 years ago

Done. Should I refactor the commits or is it ok ?

Yamakaky commented 8 years ago

One problem still: glium::Frame is stored in Renderer, so when the window is closed it's still alive. That's the error message you see. Since you can draw multiple times before the vblank, I can't find a way to easily fix it, except create it higher in the stack trace (but it breaks encapsulation). I'm thinking of it. One way would be to render all the frame in one call.

simias commented 8 years ago

As long as each commit builds individually it's fine (otherwise it's annoying when bisecting a regression). I'll review all that ASAP.

simias commented 8 years ago

Mmh, so I just built your PR and I still have increased brightness compared to master, do you know what causes that?

Yamakaky commented 8 years ago

No...

simias commented 8 years ago

Strange, do you have the same behaviour on your end or is it something on my side?

Yamakaky commented 8 years ago

I just checked, I see the correct colors.

simias commented 8 years ago

By correct colors you mean that you see the same colors with master and your branch? Could you make a screenshot so that I could compare with mine, just to see which one is right and which one is wrong.

Yamakaky commented 8 years ago

With the two versions, I get this image

Yamakaky commented 8 years ago

Should be OK for merge.

simias commented 8 years ago

Okay, I haven't figured out why the colors are brighter on my end but at this point it's not too important.

Do you think we could avoid the "The Frame object must be explicitly destroyed by calling .finish()" error on quit by adding a Drop implementation to the renderer?

Yamakaky commented 8 years ago

Not sure, as we have to drop it before the window is closed. On it.

tomaka commented 8 years ago

The colors are brighter because glium automatically enables gamma correction by default.

If your shader returns RGB colors, then the colors that you see with glium are correct. If your shader already does gamma correction (or if the playstation does not do gamma correction), then you can disable this at program creation.

Yamakaky commented 8 years ago

Then why don't we have the same results?

tomaka commented 8 years ago

If the OpenGL driver doesn't support automatic gamma correction (if you don't have GL_ARB_framebuffer_sRGB or OpenGL 3.0), then it's automatically ignored.

Normally it's not a problem since almost everyone supports it (just like everyone supports shaders and framebuffer objects for example), but I think there's some sort of bug where Mesa doesn't report it correctly.

Yamakaky commented 8 years ago

For the record, I'm on archlinux, xorg, macbook pro 8.1, intel integrated HD3000.

Yamakaky commented 8 years ago

Fixed by #17

simias commented 8 years ago

Well, that's a tricky issue actually, normally if we want to have accurate colors we should emulate the NTSC/PAL color profiles. It shouldn't be too hard to integrate a shader to do that, eventually. It's definitely not sRGB, that's for sure.

Yamakaky commented 8 years ago

Did you try #17 ?

simias commented 8 years ago

I see that in the new renderer::draw code there's no longer an explicit wait for the draw to complete, I assume that it means that glium's draw call blocks by default?

If that's the case is it possible to have a non-blocking draw for instance if we double buffer the vertex buffer to queue new vertex while the previous frame is rendering? That was an optimization I had in mind for the raw OpenGL renderer.

tomaka commented 8 years ago

draw doesn't block. The only thing that could block is Frame::finish if you have vsync enabled. But that was already the case with the previous code.

Yamakaky commented 8 years ago

Synchronisation of the permanent buffer is automatic.

simias commented 8 years ago

So I don't quite understand, how do you guarantee that the content of the vertex buffer is not overwritten while still being processed by the GPU?

tomaka commented 8 years ago

Ah I didn't see you were using PMBs.

Glium creates and manages sync fences automatically. If you use PMBs you should use triple buffering to avoid blocking.

simias commented 8 years ago

so that means three different vertex buffers right? I assume the locking is done at the vb level.

tomaka commented 8 years ago

Having a single vertex buffer that is three times the normal size also works, and is faster because you will only need one VAO.

Here is an example (with an old version of glium):

Glium will only use one VAO and will automatically use glDrawElementsBaseVertex.

simias commented 8 years ago

So glium only locks a slice of the vertex buffer? That's pretty neat.

tomaka commented 8 years ago

Yes

simias commented 8 years ago

Cool, thank you for the details.