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(engine): move ImGui::End call to outside of if body #1353

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SrGesus
Copy link
Contributor

@SrGesus SrGesus commented Oct 22, 2024

Description

Moved ImGui::End call to outside of if body.

Context

ImGui::End call was inside an if statement, meaning it would not be executed sometimes despite ImGui::Begin being called, this results in a crash.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

@SrGesus SrGesus requested review from RiscadoA and a team as code owners October 22, 2024 23:31
@SrGesus SrGesus requested review from roby2014 and RodrigoVintem and removed request for a team October 22, 2024 23:31
Copy link
Contributor

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1353/
on branch gh-pages at 2024-10-22 23:32 UTC

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 41.54%. Comparing base (86781d5) to head (6ba04f3).

Files with missing lines Patch % Lines
engine/src/tools/play_pause/plugin.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1353   +/-   ##
=======================================
  Coverage   41.54%   41.54%           
=======================================
  Files         437      437           
  Lines       32850    32850           
  Branches     3820     3820           
=======================================
  Hits        13646    13646           
  Misses      19204    19204           

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

Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

LGTM! Don't forget to add an entry to the changelog 😌

@SrGesus
Copy link
Contributor Author

SrGesus commented Oct 23, 2024

LGTM! Don't forget to add an entry to the changelog 😌

Haha I'd thought it was too little of a contribution to add it

@RiscadoA
Copy link
Member

LGTM! Don't forget to add an entry to the changelog 😌

Haha I'd thought it was too little of a contribution to add it

Still a fix though, gotta get that credit 👀

Copy link
Contributor

@RodrigoVintem RodrigoVintem left a comment

Choose a reason for hiding this comment

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

Codecov / codecov/patch

engine/src/tools/play_pause/plugin.cpp#L58

Added line #L58 was not covered by tests

Isto não será importante?

@RiscadoA
Copy link
Member

RiscadoA commented Oct 24, 2024

Codecov / codecov/patch

engine/src/tools/play_pause/plugin.cpp#L58

Added line #L58 was not covered by tests

Isto não será importante?

Bem apanhado! Infelizmente a maioria do engine não está coberto pelos testes 😔
É basicamente só porque é mesmo muito dificil testar coisas 'gráficas' automaticamente, por isso we don't really do it.
Isto é um desses casos, já que a feature de que estamos a falar é o engine crashar quando uma janela da interface de debug é escondida (iirc)

Mas yeah, de resto, quando é algo deterministico e não demasiado dificil de testar, é sempre bom tentar cobrir com testes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants