Fix macos build and run issues #1777
Conversation
|
Changing the structure of engine.loop have resulted in some new issues that need to be fixed, error output: |
For an unkown reason switch statements in the shaders were causing glLinkProgram() to crash
Skosulor
force-pushed
the
fix_macos_build_and_run_issues
branch
from
2e546ba to
e72b2e1
Compare
|
I set breakpoints around at destructors and got: Not sure how to interpret this. What can I do to debug main.qml? |
|
You can ignore the QML error. It has nothing to do with this. It only happens because our GUI is currently deactivated. |
|
I'm a bit dumbfounded, when the deconstructor for Engine is called I have the following backtrace: Which means that we have come to the end of run_game(). But I also have breakpoints inside the engine.loop function just after the presentation.run() is called. void Engine::loop() {
// if presenter is used, run the simulation in a separate thread
if (this->run_mode == mode::FULL) {
this->threads.emplace_back([&]() {
this->loop_simulation();
});
this->presenter->run(this->window_settings);
log::log(MSG(info) << "presenter exited");
// Make sure that the presenter gets destructed in the same thread
// otherwise OpenGL complains about missing contexts
this->presenter.reset();
this->running = false;
}
else {
this->loop_simulation();
}
}And the execution is never stopped inside the loop() function, nor is So it seems that the presenter never exits. Which lead me to debugging the presenter again, and it seems I ever get past this->gui = std::make_shared<renderer::gui::GUI>(
this->gui_app, // Qt application wrapper
this->window, // window for the gui
qml_root_file, // entry qml file, absolute path.
qml_root, // directory to watch for qml file changes
qml_assets, // qml data: Engine *, the data directory, ...
this->renderer // openage renderer
);Seem to halt the execution of the thread, at least I never get past it before the errors. I tried to remove main.qml to see what happens. I would expect an error thrown from: this->gui = std::make_shared<renderer::gui::GUI>(
this->gui_app, // Qt application wrapper
this->window, // window for the gui
qml_root_file, // entry qml file, absolute path.
qml_root, // directory to watch for qml file changes
qml_assets, // qml data: Engine *, the data directory, ...
this->renderer // openage renderer
);But instead I see an endless spam of errors, see the gist: https://gist.github.com/Skosulor/0ca46ceeb1c160d88ec0582cce25d1b2 It continues like that for like a couple hundred of thousand lines. I found that it is actually in the log message when getting the path: std::string Path::resolve_native_path_w() const {
auto resolved_path = this->fsobj->resolve_w(this->parts);
if (resolved_path.first) {
return resolved_path.second.get_native_path();
}
else {
throw Error{ERR << "failed to locate writable path: " << *this};
}I end up in the Anyway, it seems to me that the error I receive: Is related to the presenter failing to initialize graphics as EDIT: Nevermind The main thread does not seem to get passed the: this->gui = std::make_shared<renderer::gui::GUI>(
this->gui_app, // Qt application wrapper
this->window, // window for the gui
qml_root_file, // entry qml file, absolute path.
qml_root, // directory to watch for qml file changes
qml_assets, // qml data: Engine *, the data directory, ...
this->renderer // openage renderer
);Code inside presenter.cpp |
|
Okay I think I found it, it seems to be more shader problems. Actually crashing on: and digging deeper I found that the shader compilation fails:
As seen in the screenshot above GLShader::GLShader fails to compile the shader and for somereason macos does not like exceptions and I get the output seen in the gist earlier. To conclude my debugging I guess there are two issues
|
|
Can you comment the whole of GUI init and rendering out in the presenter? Maybe this fixes it. The GUI is not drawing anything anyway currently. openage/libopenage/presenter/presenter.cpp Line 169 in 86d05c4 and openage/libopenage/presenter/presenter.cpp Line 316 in 86d05c4 |
|
I think the issue is that the shader code is too old for the shaders in the GUI. They are based on OpenGL 1.2... |
Yup, I just found it! And you are correct, I updated them and that part works now. However now I think there are a new issue.. |
|
We should be almost there. The last renderer sytem that is initialized after the GUI is the final render pass that composes all other renderers. However, that has a very simple shader. |
heinezen
added
os: macos
|
Looks like its failing to set unform texture because it does not exist? this->texture_unif = maptex_shader->new_uniform_input("texture", this->texture);
this->texture is confirmed to have been set so I do not know how to interpret the error. |
|
@Skosulor The error refers to a uniform in the shader source code. What that basically means is: It tried to set a value for the uniform named "texture", but the GLSL source code does not have a uniform with that name. Edit: The uniform is here in the GLSL code: |
|
You might be able to upgrade the shader to OpenGL 3.3 with this code. I did not test this: #version 330
// total basic standard texture drawing fragment shader
// the texture data
uniform sampler2D texture;
// interpolated texture coordinates received from vertex shader
in vec2 tex_position;
out vec4 col;
void main (void) {
// this sets the fragment color to the corresponding texel.
col = texture2D(texture, tex_position);
} |
I made it to: So I changed it to texture_ too in the code so now I get even further! But maybe the shader should be as you have written it? But now I get a new error but I do not think it is the shader anymore! |
|
Hmm It seems a thread is trying to access a shared ptr that doesn't exist anymore |
Based on this line I think it's more likely that the time loop got destroyed somehow. |
It seems to happen because the destructor is called on most thread. I have narrowd down the crash to: for (auto &cb : this->on_resize) {
cb(width, height, this->scale_dpr);in GlWindow::Update in window.cpp:126 |
|
Eureka!
Had to update the name of texture in one more place :P |
|
Is the performance as seen in the GIF as expected from the current state of openage?
|
|
Performance as in frames? :D You can test that with stresstest 0. If the question is if that is how openage works in general, then yes, what you see looks like the correct state. |
Yeah, I meant as in frames :P It seems I cant run it
|
|
Try this |
Same result:
|
|
Looks like the docs are wrong 🙈 This command should work: |
That went fine, 1-2k FPS ish. But when launching the "full game" I experience a lot of lag when moving around the camera and poor response when zooming in and out as seen in the gif above |
|
Do the units move normally (i.e. are the animations smooth)? |
I have not figured out how to move them :P |
|
Select them with the rectangle you draw with left mouse, then right click somewhere :D |
The GIF is an accurate representation of how it looks for me |
|
But I think I'm done with the PR so it is ready for review now :) |
|
I'm going to think about a few suggestion how to structure the code and then come back with a review :) |
| << "(" << std::jthread::hardware_concurrency() << " available)"); | ||
| } | ||
|
|
||
| void Engine::loop() { |
There was a problem hiding this comment.
The structure with loop() and loop_simulation() is a bit too convoluted now I think. I recommend we organize this in a different way:
- Move all thread functions (time loop, simulation loop, presenter loop) into a separate function definition (maybe even outside
Engineclass?) - Move the thread spawning to
Engine::loop()(so that alll threads are started there) - In
Engine::loop()it should be much clearer why we are running different functions in the main thread depending on headless mode. The comments should explain that this is because of the Qt requirements.
| log::log(INFO << "Using " << this->threads.size() + 1 << " threads " | ||
| << "(" << std::jthread::hardware_concurrency() << " available)"); |
There was a problem hiding this comment.
The number is now no longer correct because simulation/presenter are started later. That's why I made the recommendation to start all threads in loop() in the other comment.
| /** | ||
| * Window settings | ||
| */ | ||
| const renderer::window_settings window_settings; | ||
|
|
There was a problem hiding this comment.
It's probably better to store them in the presenter class and pass them in the constructor.
| @@ -87,7 +88,7 @@ void GUI::initialize_render_pass(size_t width, | |||
| resources::Texture2dInfo(width, height, resources::pixel_format::rgba8)); | |||
| auto fbo = this->renderer->create_texture_target({output_texture}); | |||
|
|
|||
| this->texture_unif = maptex_shader->new_uniform_input("texture", this->texture); | |||
| this->texture_unif = maptex_shader->new_uniform_input(texture_, this->texture); | |||
There was a problem hiding this comment.
Using a static variable should not be necessary for new_uniform_input(..), since all other calls just pass an in-place string. You should just be able to pass the string without any problems.
There was a problem hiding this comment.
I created a constexpr as "texture" was used in more than one place. IMO it is better to have a constexpr/define rather than a hardcoded value, especially if it is used in more than one place.
However if you rather want it hardcoded I will object to it.
There was a problem hiding this comment.
I would agree in general, but for these WIP parts of the engine it's probably better to hardcode it for now. My reason would be that it then becomes more "visible" what is done with the shader until it's properly defined. For the more mature shaders, we have a solution that is similar to what you propose where we store the uniform ID and use that
openage/libopenage/renderer/stages/world/object.h
Lines 147 to 157 in 86d05c4
|
|
||
| // the texture data | ||
| uniform sampler2D texture; | ||
| uniform sampler2D texture_; |
There was a problem hiding this comment.
Better rename it tex than using underscores because that aligns with other shader source code definitions.
| uniform sampler2D texture_; | |
| uniform sampler2D tex; |
| // this sets the fragment color to the corresponding texel. | ||
| gl_FragColor = texture2D(texture, tex_position); | ||
| void main(void) { | ||
| frag_color = texture(texture_, tex_position); |
There was a problem hiding this comment.
| frag_color = texture(texture_, tex_position); | |
| frag_color = texture(tex, tex_position); |
|
|
||
| void main(void) { | ||
| gl_Position = vertex_position; | ||
| gl_Position = vertex_position; |
There was a problem hiding this comment.
afaik you don't need this statement since gl_Position by definition contains the current vertex position already. You can remove that line.
| gl_Position = vertex_position; |
| if (alpha == 0) { | ||
| col = tex_val; | ||
| discard; | ||
| return; | ||
| } else if (alpha == 254) { | ||
| col = vec4(1.0f, 0.0f, 0.0f, 1.0f); | ||
| } else if (alpha == 252) { | ||
| col = vec4(0.0f, 1.0f, 0.0f, 1.0f); | ||
| } else if (alpha == 250) { | ||
| col = vec4(0.0f, 0.0f, 1.0f, 1.0f); | ||
| } else { | ||
| col = tex_val; |
There was a problem hiding this comment.
switch statements should definitely work in this place, so I would revisit this shader once you have fixed all the other comments.
| if (alpha == 0) { | ||
| discard; | ||
| } else if (alpha == 254) { | ||
| col = vec4(1.0, 0.0, 0.0, 1.0); | ||
| } else if (alpha == 252) { | ||
| col = vec4(0.0, 1.0, 0.0, 1.0); | ||
| } else if (alpha == 250) { | ||
| col = vec4(0.0, 0.0, 1.0, 1.0); | ||
| } else { |
| if (alpha == 0) { | ||
| discard; | ||
| } else if (alpha == 254) { | ||
| col = vec4(1.0, 0.0, 0.0, 1.0); | ||
| } else if (alpha == 252) { | ||
| col = vec4(0.0, 1.0, 0.0, 1.0); | ||
| } else if (alpha == 250) { | ||
| col = vec4(0.0, 0.0, 1.0, 1.0); | ||
| } else { |
|
Thanks! I will rework the PR according to your comments. Will be away for a few days so there might be no progress until next week. |
The goal of this pull request is to fix build and run issues for macOs.
Note: Requires some additional tweeks