Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

opengl3 driver now works on windows including multi window #60894

Merged
merged 1 commit into from
May 13, 2022
Merged

opengl3 driver now works on windows including multi window #60894

merged 1 commit into from
May 13, 2022

Conversation

derammo
Copy link
Contributor

@derammo derammo commented May 8, 2022

fixed and simplified gl_manager_windows
swap buffers now called for all windows
fixed missing pixel format setting in additional windows
this makes them work in OpenGL contexts
changed verbose error printing to write once
this error message happens very frequently while opengl3 is not finished
removed dead code no longer needed after changes
fixed comments that were misinformation
window messages during window creation now handled
these were previously discarded
messages now tunnel the required context
changed failure to create opengl3 window on windows to be more fatal
marked a problem with pen code
conditional compilation of vulkan and opengl3 on windows fixed
windows debug builds now show messages on debug console also
rendering driver selection box now shows only compiled drivers
marked some problematic code

thanks to @clayjohn for on-boarding and help/collaboration

@derammo derammo requested review from a team as code owners May 8, 2022 23:35
@Calinou Calinou added this to the 4.0 milestone May 8, 2022
@derammo
Copy link
Contributor Author

derammo commented May 8, 2022

I will amend and force push to get rid of the compilation issues.

clayjohn
clayjohn reviewed May 9, 2022
View reviewed changes
drivers/gles3/rasterizer_gles3.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
platform/windows/display_server_windows.cpp Outdated Show resolved Hide resolved
platform/windows/vulkan_context_win.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_scene_cull.cpp Outdated Show resolved Hide resolved
platform/windows/detect.py Show resolved Hide resolved
platform/windows/display_server_windows.cpp Outdated Show resolved Hide resolved
@derammo derammo force-pushed the derammo_opengl3_windows branch 2 times, most recently from cd0ff93 to a9e533c Compare May 9, 2022 00:23
@derammo
Copy link
Contributor Author

derammo commented May 9, 2022

I believe that I have resolved the static and compilation issues. Do I just wait for someone to kick those test builds again and then I get email if they fail?

@clayjohn
Copy link
Member

clayjohn commented May 9, 2022

I believe that I have resolved the static and compilation issues. Do I just wait for someone to kick those test builds again and then I get email if they fail?

Pretty much yeah. This just happens on your first PR. Once this is merged tests will run automatically for you.

@derammo
Copy link
Contributor Author

derammo commented May 9, 2022

Pretty much yeah. This just happens on your first PR. Once this is merged tests will run automatically for you.

well at least I have learned that having clang-format on a file saving trigger is pretty much required here....

@derammo derammo force-pushed the derammo_opengl3_windows branch from a9e533c to c8c03d5 Compare May 9, 2022 01:11
clayjohn
clayjohn reviewed May 9, 2022
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added one more comment, but beyond that this looks good to go!

bruvzg
bruvzg reviewed May 9, 2022
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, changes seems good.

@derammo derammo force-pushed the derammo_opengl3_windows branch from c8c03d5 to fea5eba Compare May 10, 2022 01:49
@clayjohn clayjohn requested a review from bruvzg May 11, 2022 14:51
@derammo derammo force-pushed the derammo_opengl3_windows branch from fea5eba to 881ab4f Compare May 11, 2022 16:10
@derammo
Copy link
Contributor Author

derammo commented May 11, 2022 •
edited
Loading

@bruvzg thanks a lot for all your time. I think it is ready for final review now

akien-mga
akien-mga reviewed May 11, 2022
View reviewed changes
main/main.cpp Outdated Show resolved Hide resolved
@derammo derammo mentioned this pull request May 11, 2022
Closed
akien-mga
akien-mga reviewed May 11, 2022
View reviewed changes
main/main.cpp Outdated Show resolved Hide resolved
@derammo
opengl3 driver now works on windows including multi window
96c21bc 
fixed and simplified gl_manager_windows
swap buffers now called for all windows
fixed missing pixel format setting in additional windows
    this makes them work in OpenGL contexts
changed verbose error printing to write once
    this error message happens very frequently while opengl3 is not finished
removed dead code no longer needed after changes
fixed comments that were misinformation
window messages during window creation now handled
    these were previously discarded
    messages now tunnel the required context
changed failure to create opengl3 window on windows to be more fatal
marked a problem with pen code
conditional compilation of vulkan and opengl3 on windows fixed
windows debug builds now show messages on debug console also
rendering driver selection box now shows only compiled drivers
marked some problematic code
thanks to akien-mga for patiently rewriting my style mistakes
@derammo derammo force-pushed the derammo_opengl3_windows branch from a1b21eb to 96c21bc Compare May 11, 2022 20:20
@derammo
Copy link
Contributor Author

derammo commented May 11, 2022

@akien-mga I believe I corrected all your blockers.

@akien-mga
Copy link
Member

akien-mga commented May 11, 2022 •
edited
Loading

Thanks, I'll re-review tomorrow. I apologize if my review comments were frustrating, this was not my intention. Godot is a big codebase with a lot of written and unwritten conventions which make a project with 1600+ co-authors possible and maintainable by the few dozens who stick around long term. I can understand that upholding these requirements on things which are not critical to what the PR does can be annoying.

I'd like to clarify that all review comments are not blockers. It's a discussion, and some of the suggestion I or others make may or may not be good. If there are good reasons not to follow them (e.g. the goto issue above) then that's perfectly fine.

@derammo
Copy link
Contributor Author

derammo commented May 11, 2022

@akien-mga I thank you for your kind words. Yes, you did make me decide to quit since I don't want to spend my time on minutiae, as I am old and grumpy. I only started last week but I think I will give my relationship with godot another chance based on your clarification.

akien-mga
akien-mga approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll let @bruvzg review for the Windows bits and @clayjohn for OpenGL.

clayjohn
clayjohn approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my end!

@akien-mga akien-mga merged commit 349aa9c into godotengine:master May 13, 2022
@akien-mga
Copy link
Member

Thanks!

@iwangxiaodong iwangxiaodong mentioned this pull request Jun 4, 2022
Closed
@Knand1861
Copy link

With godot.4.11a problem is back.

@Calinou
Copy link
Member

Calinou commented Jul 11, 2022

@Knand1861 Please don't bump multiple issues/pull requests for this. Instead, continue the discussion in #57180.

@derammo derammo deleted the derammo_opengl3_windows branch September 15, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruvzg bruvzg bruvzg left review comments

@akien-mga akien-mga akien-mga approved these changes

@clayjohn clayjohn clayjohn approved these changes

Assignees
No one assigned
Labels
bug platform:windows topic:porting topic:rendering
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@derammo @clayjohn @akien-mga @Knand1861 @Calinou @bruvzg