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

Replace typeid by reflection on asset bridges #795

Merged
merged 5 commits into from
Nov 18, 2023
Merged

Replace typeid by reflection on asset bridges #795

merged 5 commits into from
Nov 18, 2023

Conversation

roby2014
Copy link
Member

@roby2014 roby2014 commented Nov 11, 2023 •
edited
Loading

Blocked by #799 and uses commits from #775

Description

  • Replace typeid by reflection on asset bridges
  • Fixed tesseratos plugins where asset types are compared
  • Fixed assets samples to use this new system

Checklist

  • Compiled and executed all assets samples

@roby2014 roby2014 linked an issue Nov 11, 2023 that may be closed by this pull request
Replace typeid by reflection on asset bridges #581
Closed
@roby2014 roby2014 self-assigned this Nov 11, 2023
@roby2014 roby2014 added A-Engine B-Assets C-Code-Quality A section of code that is hard to understand or change B-Reflection labels Nov 11, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 11, 2023 •
edited
Loading

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-11-18 13:54 UTC

github-actions[bot]
github-actions bot previously requested changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

engine/include/cubos/engine/assets/assets.hpp Outdated Show resolved Hide resolved
engine/src/cubos/engine/assets/assets.cpp Outdated Show resolved Hide resolved
engine/src/cubos/engine/assets/assets.cpp Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Nov 11, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c8f5985) 41.79% compared to head (483b100) 41.79%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #795   +/-   ##
=======================================
  Coverage   41.79%   41.79%           
=======================================
  Files         108      108           
  Lines        7005     7005           
=======================================
  Hits         2928     2928           
  Misses       4077     4077

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

RiscadoA
RiscadoA requested changes Nov 11, 2023
View reviewed changes
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.

Thanks for working on this @roby2014! Didn't realize it also meant changing the internals of the Assets resource 😔

engine/include/cubos/engine/assets/assets.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/assets.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridge.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/binary.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridge.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/file.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/json.hpp Show resolved Hide resolved
engine/include/cubos/engine/scene/bridge.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/scene/bridge.hpp Outdated Show resolved Hide resolved
@roby2014 roby2014 added the S-Blocked Blocked on another issue or PR label Nov 11, 2023
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch from f4e53bf to f3f05cf Compare November 11, 2023 12:33
@github-actions github-actions bot dismissed their stale review November 11, 2023 12:35

No Clang-Tidy warnings found so I assume my comments were addressed

@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch 6 times, most recently from f4bf19a to 9fc2633 Compare November 11, 2023 14:22
github-actions[bot]
github-actions bot previously requested changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

engine/include/cubos/engine/scene/bridge.hpp Outdated Show resolved Hide resolved
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch 2 times, most recently from bedc510 to b81194b Compare November 11, 2023 14:29
@github-actions github-actions bot dismissed their stale review November 11, 2023 14:30

No Clang-Tidy warnings found so I assume my comments were addressed

@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch 2 times, most recently from 429203b to 48e58b0 Compare November 11, 2023 14:38
@roby2014 roby2014 marked this pull request as ready for review November 11, 2023 14:43
@roby2014 roby2014 requested a review from RiscadoA November 11, 2023 14:44
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch from 48e58b0 to 93d3654 Compare November 12, 2023 18:28
@roby2014 roby2014 mentioned this pull request Nov 12, 2023
Merged
4 tasks
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch from 93d3654 to 3c9c044 Compare November 13, 2023 05:34
RiscadoA
RiscadoA approved these changes Nov 13, 2023
View reviewed changes
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.

Thanks for working on this! Overall LGTM, but please remove that const_cast - types should never be referenced in a mutable way, only during their creation do we provide mutable access.

engine/include/cubos/engine/assets/assets.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/assets.hpp Outdated Show resolved Hide resolved
engine/src/cubos/engine/assets/assets.cpp Outdated Show resolved Hide resolved
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch 3 times, most recently from 449e834 to 85b5c7c Compare November 13, 2023 22:31
@roby2014 roby2014 mentioned this pull request Nov 18, 2023
Merged
4 tasks
@roby2014 roby2014 removed the S-Blocked Blocked on another issue or PR label Nov 18, 2023
@roby2014 roby2014 force-pushed the 581-replace-typeid-by-reflection-on-asset-bridges branch from 85b5c7c to 483b100 Compare November 18, 2023 13:27
@roby2014 roby2014 merged commit d7cf365 into main Nov 18, 2023
@roby2014 roby2014 deleted the 581-replace-typeid-by-reflection-on-asset-bridges branch November 18, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RiscadoA RiscadoA RiscadoA approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@DiogoMendonc-a DiogoMendonc-a Awaiting requested review from DiogoMendonc-a

@luishfonseca luishfonseca Awaiting requested review from luishfonseca

Assignees

@roby2014 roby2014

Labels
A-Engine B-Assets B-Reflection C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace typeid by reflection on asset bridges
2 participants
@roby2014 @RiscadoA