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.

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

Fix crash when using ImGui across DLL boundaries on Windows #1376

Conversation

RiscadoA
Copy link
Member

@RiscadoA RiscadoA commented Nov 21, 2024

Description

You can read the documentation I included in the PR changes to understand what I'm fixing here.
If you're running Windows, previously with CUBOS_ENGINE_SHARED set to ON on CMake you probably got a crash on both Tesseratos and on the ImGui sample. With this change, that should no longer happen, so please test that before approving.

Checklist

  • Self-review changes.
  • Updated imgui plugin documentation.
  • Update sample.
  • Add entry to the changelog's unreleased section.

@RiscadoA RiscadoA added this to the 0.5 milestone Nov 21, 2024
@RiscadoA RiscadoA self-assigned this Nov 21, 2024
@RiscadoA RiscadoA added the O-Windows Related to Windows builds label Nov 23, 2024
@RiscadoA RiscadoA force-pushed the 1144-imguisetcurrentcontext-should-be-called-on-all-dllsapps-that-use-it branch from b8fee04 to 549b617 Compare November 23, 2024 10:01
@RiscadoA RiscadoA marked this pull request as ready for review November 23, 2024 10:01
@RiscadoA RiscadoA requested review from a team as code owners November 23, 2024 10:01
@RiscadoA RiscadoA requested review from jdbaracho, SrGesus and diogomsmiranda and removed request for a team November 23, 2024 10:01
Copy link
Contributor

github-actions bot commented Nov 23, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1376/
on branch gh-pages at 2024-11-30 08:27 UTC

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 52.92%. Comparing base (ddfe645) to head (d6e3f46).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
engine/src/imgui/plugin.cpp 0.00% 8 Missing ⚠️
engine/src/imgui/context.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1376      +/-   ##
==========================================
- Coverage   52.94%   52.92%   -0.02%     
==========================================
  Files         461      462       +1     
  Lines       26084    26090       +6     
  Branches     2419     2419              
==========================================
  Hits        13809    13809              
- Misses      12275    12281       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@diogomsmiranda
Copy link
Contributor

If @SrGesus and @jdbaracho are Windows users then I don't mind looking at the code and review it, else I think we should assign Windows users as reviewers no? I have no machine to test this on.

@RiscadoA thoughts?

Copy link
Contributor

@diogomsmiranda diogomsmiranda left a comment

Choose a reason for hiding this comment

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

LGTM!

engine/samples/imgui/page.md Outdated Show resolved Hide resolved
@jdbaracho
Copy link
Contributor

jdbaracho commented Nov 29, 2024

Is this about CUBOS_ENGINE_SHARED or BUILD_ENGINE_SHARED @RiscadoA ?

@RiscadoA
Copy link
Member Author

Is this about CUBOS_ENGINE_SHARED or BUILD_ENGINE_SHARED @RiscadoA ?

The first one, the second one no longer exists, it was the old name of the same option which was renamed in a previous PR

Copy link
Contributor

@jdbaracho jdbaracho left a comment

Choose a reason for hiding this comment

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

Tested on a Windows machine, at least the ImGUI sample and cubosurfers seem to run without any problems.
LGTM!

@RiscadoA RiscadoA force-pushed the 1144-imguisetcurrentcontext-should-be-called-on-all-dllsapps-that-use-it branch from 549b617 to d6e3f46 Compare November 30, 2024 08:26
@RiscadoA RiscadoA enabled auto-merge (rebase) November 30, 2024 08:26
@RiscadoA RiscadoA merged commit c274628 into main Nov 30, 2024
9 checks passed
@RiscadoA RiscadoA deleted the 1144-imguisetcurrentcontext-should-be-called-on-all-dllsapps-that-use-it branch November 30, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImGui::SetCurrentContext should be called on all DLLs/apps that use it
4 participants