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

BREAKING CHANGE: ecstasy::ResourceReference now use std::reference_wrapper #177

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

AndreasLrx
Copy link
Owner

@AndreasLrx AndreasLrx commented Oct 18, 2024

Description

TL;DR;

Old behavior:

class MyResource : public ecstasy::Resource<MyResource> {
   [...]
};

/* Inside a function */
registry.getResource<MyResource>()->myMethod();

New behavior:

class MyResource : public ecstasy::IResource {
   [...]
};

/* Inside a function */
registry.getResource<MyResource>().get().myMethod();

Explanations

And added a get method in the LockableView, equivalent to the one of the std::reference_wrapper. This means all ResourceReference need to use get() to access the underlying resource, regardless of the thread safe variable.

The previous way was to always use the arrow operator ->, which was defined in the templated Resource class but it wasn't a good idea because it breaks when the underlying type also define the -> arrow operator (such as ObjectWrapper, which was used by the sfml and was causing the initial issue #170).
Therefore this behavior was deleted: Resource<> and ResourceBase are now a single basic IResource with nothing in it (still required as a type erasure way to store the resources in the same container)

Close #170

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

[optional] Minor fixes

Fix #174 not running build CI AT ALL

Added/updated tests?

  • New tests written
  • Existing tests updated
  • Tests are not required because this is a documentation update
  • I need help with writing tests

@AndreasLrx AndreasLrx added bug Something isn't working BREAKING CHANGE This may break existing user code labels Oct 18, 2024
@AndreasLrx AndreasLrx self-assigned this Oct 18, 2024
…apper when not thread safe

Also removed the templated ecstasy::Resource and base ecstasy::ResourceBase classes, both replaced by the simplier IResource base class.

Close #170
@AndreasLrx AndreasLrx force-pushed the fix/170-sfml-integration-compilation branch from 8bd8928 to 2b567f3 Compare October 18, 2024 14:33
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 82.01%. Comparing base (2706640) to head (2b567f3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/ecstasy/resources/IResource.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   81.93%   82.01%   +0.08%     
==========================================
  Files          64       64              
  Lines        1129     1129              
  Branches      352      352              
==========================================
+ Hits          925      926       +1     
+ Misses        144      143       -1     
  Partials       60       60              

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

@AndreasLrx AndreasLrx merged commit 068db78 into main Oct 18, 2024
4 checks passed
@AndreasLrx AndreasLrx deleted the fix/170-sfml-integration-compilation branch October 18, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This may break existing user code bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] SFML integration compilation fails without ECSTASY_THREAD_SAFE
2 participants