Closed mrmixer closed 2 months ago
Looks good, and seems to work fine on my Nvidia card as well, would be good to get some feedback from other AMD users though. I see you also have another pr open, does this PR resolve that issue as well, or is it still relevant?
Yes, this PR contains the other fix. I just pushed a small change to use GL_TRUE instead of true, just to use the "official" OpenGL wording.
I just pushed a fix for an issue (in the directx code) where 4coder crashes when we try to render more vertex than what I expected a good limit would be. Now it will try to allocate a new vertex buffer (this is somewhat similar to what OpenGL is doing) with a size big enough to render, and if it fails, will skip the render group that is too big (in practice it means that the panel containing the file will be empty). The creation of the buffer is not likely to fail as modern graphics cards have a lot of memory and we are only requesting a fraction of it (~11Mio by default).
I encounter that issue because 4coder appears to not clip text that is outside the screen horizontally, and I have a header that contains a big array that represent image data, and 4coder doesn't wrap that. Adding clipping of characters outside the screen would be a good idea as the performances aren't great when rendering that much characters.
u8 data[ ] = {0,0,0,0,0,...0}; // Will not wrap with no space in the list.
For the testing, do you think I could @ everybody on the 4coder discord to ask for some testing ? I'm not very used to discord so I don't know if that's frowned upon ?
Do you want testing of the D3D11 version or the OpenGL version to make sure it still functioning properly ? I'm assuming we want both, but what is the priority ? Do you (we ?) intend to make the D3D11 version the default ? I don't expect a definitive answer, but your thoughts on this would be appreciated. My reason for D3D11 being the default was that it solved a small issue on an AMD card (fullscreen alt tabbing causing a black flash), but it might just be an issue on my computer as far as I know, so it's not really a good argument. If other AMD user have a similar issue (I would ask testers), it might be worth it, as there shouldn't be other differences between the OpenGL and D3D11 versions. Also remember that this pull request contains a fix for OpenGL on AMD cards.
In relation to that, do you intend to have "official" binary releases of the community fork ? Or do we expect users that want updates just download the source and build 4coder themselves ? If no binary release, then the default matters less, as we can expect the user to change the defines to get the version they want.
It looks like this version doesn't corrently throttle the framerate. If you run the toggle_fps_meter
command on master, when inactive 4coder will throttle to 1fps, however that doesn't happen here.
Sorry for the late reply.
On my side, 4coder throttles to 1fps correctly, in both fullscreen and windowed. I tried my custom layer and the default build, and can't see any issue.
I wouldn't expect the changes from this pull request to change anything relating to frame timing and presentation (but it could be a undesired side effect). The code requests to wait for vblank to present (as OpenGL did) and if I remember correctly, 4coder uses GetMessage
to wait for messages, and when it gets a message, it uses PeekMessage
to empty the message queue and then goes back to GetMessage
. I suppose there is a timer/user message to force an update every seconds (I didn't check but that's what I would expect).
My guess (I haven't looked at the code yet) is that you're skipping the GetMessage part. I'll have a look at what could cause that when I've got some time.
To keep this up to date: I had two persons testing the issue, both on Windows 11, one on NVidia RTX3080 and one on AMD RX7800 and they didn't have the issue.
Sorry for the delay. I did some more digging and I also can't reproduce the issue, not sure what I was seeing. I'm wondering if we want to merge this as is, or if we should maybe take the time to rename the API so its not gl specific. I'm not super hot on doing graphics API code, so I'm not totally sure what this should look like for a API that is graphics API agnostic. It's probably also worth making the DX11 flag a build option and to put a note in the readme with how to swap. between the two
if we should maybe take the time to rename the API so its not gl specific.
Do you mean not using OpenGL parlance, like "bind texture" "create program" ?
The only graphics API related thing that gets called outside the backend is texture. And there is already graphics_get_texture/graphics_fill_texture as an API for that. And the only code outside of the render backend to use those is the font provider (if I remember correctly).
I don't think it's important or useful to define more API unless we have specific use case that requires it. At the moment 4coder only render one type of primitive, using a single shader, and it makes it simple to add other backends with very little constraint.
It's probably also worth making the DX11 flag a build option and to put a note in the readme with how to swap. between the two
I didn't want to make that at the time, because the build system is confusing enough. But I could take some time to try and do that.
I spent some time today to add parameters to the build system to be able to choose between OpenGL and Direct3D11.
I don't understand the batch loop syntax, so checking for arguments value is a bit messy.
I updated the README file to give some indications on how to choose between the backends. I also added the list of possible parameters to the build system. I don't know if we should keep that there, but it's annoying enough to figure out what the build system does, so that information should be kept somewhere. Also do we want to remove the demo
from the build system ? And does the 32bits versions still work ?
And I didn't test on Linux and Mac. Batch files don't matter, but there are some modifications to 4ed_build.cpp
.
I didn't want to make that at the time, because the build system is confusing enough. But I could take some time to try and do that. Thanks for taking the time to do that :)
Looks good, I think i got you on the parameter looping thing, although im not sure its necessary as its all going into one parameter anyway so can you just unpack all the arguments into opts?
What I meant by abstarcting the api is that we call a win32_gl_create_window
even in DX, its a pretty minor coment though, not the end of the world.
Also do we want to remove the demo from the build system ? And does the 32bits versions still work?
I believe the 32-bit versions still work, they should do anyway, i just removed all but the PACKAGE_SUPER form package.bat because we dont use the other bulid types. We definitely should remove the demo versions, they're absolutely not needed anymore. We can probably get rid of the 32bit ones as well, tbh idk how useful they are. Can do that seprately though.
I think one more pass on that bat file and this is good to go.
What I meant by abstarcting the api is that we call a win32_gl_create_window even in DX, its a pretty minor coment though, not the end of the world.
For me here "gl" means "graphics layer" or "graphics lib", it doesn't mean OpenGL. We could rename it to just graphics
(like win32_graphics_create_window
if you'd like, let me know what you want.
Looks good, I think i got you on the parameter looping thing, although im not sure its necessary as its all going into one parameter anyway so can you just unpack all the arguments into opts?
Thanks for the loop but I'd prefer not to have that in the batch file. The simpler the batch file, the better.
I don't know how to write the parameters differently, (unless we use the loop). If you've got a better way let me know. I rewrote it a bit to make it clearer (at least for me) what's going on.
set mode=%1
set backend=%1
if "%1%" == "/DWIN32_OPENGL" (
set mode=%2
) else if "%1%" == "/DWIN32_DX11" (
set mode=%2
) else (
set backend=%2
)
if "%backend%" == "" (set backend=/DWIN32_OPENGL)
if "%mode%" == "" (set mode=/DDEV_BUILD)
After the conversation with Allen yesterday during their stream, I also documented the DO_CRAZY_EXPENSIVE_ASSERTS
macro in the readme. Note that only the windows build defines that macro (which isn't used anywhere at the moment), but also that it's tied to the DEBUG_INFO flag, so it's defined in packaged builds. We should fix that one day (since it's not used, it's not a problem currently).
For me here "gl" means "graphics layer" or "graphics lib", it doesn't mean OpenGL
Fair enough, as i say, its a minor thing, lets just leave it
Thanks for the loop but I'd prefer not to have that in the batch file
Tbf i agree, and tbh the build system probably needs to be completely rethought in the future anyway so this is fine imo
Nice work with the extra documentation i'll merge this :)
This branch adds a DirectX 11 backend to 4coder. There are no added functionality and the resulting render in my tests was exactly the same.
The DirectX 11 backend is useful to solve an issue with some (all ?) AMD graphics cards, where alt tabbing to another application while 4coder is in fullscreen causes a black flash (looking like a graphic mode switch). I observed this behavior with other OpenGL applications, and on two different AMD machines, and couldn't find a solution without using at least DXGI to present frames.
This branch doesn't replace OpenGL with DirectX. Both are still working, but the user need to choose at compile time which backend to use by defining
WIN32_DX11
. If the macro isn't defined OpenGL will be compiled. I placed the define for that macro incode/platform_win32/win32_4ed.cpp
and it's commented so that with the current commit, the OpenGL backend is the one used. I'm not sure how we (assuming this is merged) want to define the macro. Do we want to leave it in that file or use the build system to define it ?I changed one thing in the build system: I removed
OpenGL32.lib
from the libs passed on the command line to the compiler and replaced it with a#pragma comment(lib, "OpenGL32.lib")
in the source file. There are similar directive for directx. This is to avoid having to modify the build system.I only got one other person (on discord) to confirm that the modifications are working (on an NVidia card). More testing would be welcome.
code/opengl
directory.