-
Notifications
You must be signed in to change notification settings - Fork 12
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
Renderbackend: Rework Canvas 2D-Rendering #244
Changes from 39 commits
3362bbf
5381835
1e4cfff
683a8bd
a0202a0
1ffb692
ce8090d
a13930f
883ada3
29dcb52
4c578ff
3e79c55
037efdd
8e08167
617083b
ed8f8f3
6b0bef5
aa8725c
516cdbf
e825f62
789a398
b4c72a8
891c945
1a25271
5d4e316
231a3f6
bc1f0e8
c6f53b1
a859d0e
4ddd758
135975c
b2fdcdf
d0f4dbd
5bbcacf
7678db4
7023c4e
2c756c7
ddfe258
3d6de71
6325e47
85e6e67
edbde48
9bf581d
4f5557d
f91a4f9
2e927c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
+2 −1 | CMakeLists.txt | |
+64 −0 | doc/Common.md | |
+32 −0 | doc/Memory.md | |
+3 −1 | doc/index.md | |
+30 −26 | include/cppcore/Common/Hash.h | |
+6 −4 | include/cppcore/Container/THashMap.h | |
+3 −3 | include/cppcore/IO/FileSystem.h | |
+1 −1 | include/cppcore/Memory/TDefaultAllocator.h | |
+2 −2 | include/cppcore/Memory/TPoolAllocator.h | |
+161 −0 | include/cppcore/Memory/TScratchAllocator.h | |
+4 −2 | include/cppcore/Memory/TStackAllocator.h | |
+1 −3 | test/Random/RandomGeneratorTest.cpp | |
+16 −13 | test/common/HashTest.cpp | |
+66 −0 | test/memory/TScratchAllocatorTest.cpp | |
+1 −3 | test/memory/TStackAllocatorTest.cpp |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -40,7 +40,10 @@ bool UIElementsWin32::init() { | |||||||||||||||||||||
|
||||||||||||||||||||||
// needed for the RichEdit control in the about/help dialog | ||||||||||||||||||||||
lib = LoadLibrary("riched20.dll"); | ||||||||||||||||||||||
osre_assert(lib != nullptr); | ||||||||||||||||||||||
if (lib == nullptr) { | ||||||||||||||||||||||
osre_assert(lib != nullptr); | ||||||||||||||||||||||
return false; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling with detailed failure information. While the added error handling is an improvement, consider enhancing it with more detailed error reporting: lib = LoadLibrary("riched20.dll");
if (lib == nullptr) {
+ DWORD error = GetLastError();
+ osre_error("Failed to load riched20.dll: error code %lu", error);
osre_assert(lib != nullptr);
return false;
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
// load windows common controls library to get XP style | ||||||||||||||||||||||
InitCommonControls(); | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -40,6 +40,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||||||||||||
#include "RenderBackend/Pipeline.h" | ||||||||||||||
#include "RenderBackend/RenderBackendService.h" | ||||||||||||||
#include "RenderBackend/TransformMatrixBlock.h" | ||||||||||||||
#include "RenderBackend/2D/CanvasRenderer.h" | ||||||||||||||
#include "App/CameraComponent.h" | ||||||||||||||
#include "RenderBackend/MaterialBuilder.h" | ||||||||||||||
|
||||||||||||||
|
@@ -83,6 +84,7 @@ AppBase::AppBase(i32 argc, const c8 *argv[], const String &supportedArgs, const | |||||||||||||
mMouseEvListener(nullptr), | ||||||||||||||
mKeyboardEvListener(nullptr), | ||||||||||||||
mIds(nullptr), | ||||||||||||||
mCanvasRenderer(nullptr) { | ||||||||||||||
mStageMode(StageMode::Stage3D), | ||||||||||||||
mShutdownRequested(false) { | ||||||||||||||
mSettings->setString(Properties::Settings::RenderAPI, "opengl"); | ||||||||||||||
|
@@ -133,7 +135,7 @@ void AppBase::update() { | |||||||||||||
if (mAppState == State::Created) { | ||||||||||||||
mAppState = State::Running; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
onUpdate(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
@@ -240,6 +242,21 @@ void AppBase::setWindowsTitle(const String &title) { | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
RenderBackend::CanvasRenderer* AppBase::getCanvasRenderer() const { | ||||||||||||||
return (CanvasRenderer*) mCanvasRenderer; | ||||||||||||||
} | ||||||||||||||
Comment on lines
+242
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use static_cast instead of C-style cast. For better type safety in modern C++, prefer - return (CanvasRenderer*) mCanvasRenderer;
+ return static_cast<CanvasRenderer*>(mCanvasRenderer); 📝 Committable suggestion
Suggested change
|
||||||||||||||
|
||||||||||||||
static void attachMouseEventPtrs(EventPtrArray &eventArray) { | ||||||||||||||
eventArray.add(&MouseButtonDownEvent); | ||||||||||||||
eventArray.add(&MouseButtonUpEvent); | ||||||||||||||
eventArray.add(&MouseMoveEvent); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static void attachKeyboardEventPtrs(EventPtrArray &eventArray) { | ||||||||||||||
eventArray.add(&KeyboardButtonDownEvent); | ||||||||||||||
eventArray.add(&KeyboardButtonUpEvent); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
bool AppBase::onCreate() { | ||||||||||||||
if (mAppState != State::Uninited) { | ||||||||||||||
osre_debug(Tag, "AppBase::State not in expected state: Uninited."); | ||||||||||||||
|
@@ -295,7 +312,7 @@ bool AppBase::onCreate() { | |||||||||||||
|
||||||||||||||
// enable render-back-end | ||||||||||||||
RenderBackend::CreateRendererEventData *data = new CreateRendererEventData(mPlatformInterface->getRootWindow()); | ||||||||||||||
data->m_pipeline = mRbService->createDefault3DPipeline(); | ||||||||||||||
data->Pipeline = mRbService->createDefault3DPipeline(); | ||||||||||||||
mRbService->sendEvent(&RenderBackend::OnCreateRendererEvent, data); | ||||||||||||||
|
||||||||||||||
mTimer = Platform::PlatformInterface::getInstance()->getTimer(); | ||||||||||||||
|
@@ -323,6 +340,14 @@ bool AppBase::onCreate() { | |||||||||||||
|
||||||||||||||
App::AssetRegistry::registerAssetPathInBinFolder("assets", "assets"); | ||||||||||||||
|
||||||||||||||
Rect2ui rect; | ||||||||||||||
mPlatformInterface->getRootWindow()->getWindowsRect(rect); | ||||||||||||||
mCanvasRenderer = new CanvasRenderer(2, (i32)rect.getX1(), (i32)rect.getY1(), (i32)rect.getWidth(), (i32)rect.getHeight()); | ||||||||||||||
Check warning Code scanning / CodeQL Resource not released in destructor Warning
Resource mCanvasRenderer is acquired by class AppBase but not released in the destructor. It is released from onDestroy on line 353, so this function may need to be called from the destructor.
|
||||||||||||||
if (!mCanvasRenderer->create()) { | ||||||||||||||
osre_error(Tag, "Error while creating the canvas renderer."); | ||||||||||||||
return false; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
mAppState = State::Created; | ||||||||||||||
osre_debug(Tag, "Set application state to Created."); | ||||||||||||||
|
||||||||||||||
|
@@ -361,6 +386,9 @@ bool AppBase::onDestroy() { | |||||||||||||
delete mIds; | ||||||||||||||
mIds = nullptr; | ||||||||||||||
|
||||||||||||||
delete mCanvasRenderer; | ||||||||||||||
mCanvasRenderer = nullptr; | ||||||||||||||
|
||||||||||||||
Comment on lines
+375
to
+377
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add null check and proper cleanup call before deletion. For safer cleanup, add a null check and call the destroy method before deletion. - delete mCanvasRenderer;
- mCanvasRenderer = nullptr;
+ if (mCanvasRenderer != nullptr) {
+ mCanvasRenderer->destroy();
+ delete mCanvasRenderer;
+ mCanvasRenderer = nullptr;
+ }
|
||||||||||||||
delete mMouseEvListener; | ||||||||||||||
mMouseEvListener = nullptr; | ||||||||||||||
|
||||||||||||||
|
@@ -412,3 +440,4 @@ void AppBase::getResolution(ui32 &width, ui32 &height) { | |||||||||||||
|
||||||||||||||
} // Namespace App | ||||||||||||||
} // Namespace OSRE | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -50,6 +50,9 @@ namespace Properties { | |||||||||||||||||||
|
||||||||||||||||||||
namespace RenderBackend { | ||||||||||||||||||||
struct TransformMatrixBlock; | ||||||||||||||||||||
struct IRenderPath; | ||||||||||||||||||||
|
||||||||||||||||||||
class CanvasRenderer; | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
namespace App { | ||||||||||||||||||||
|
@@ -62,20 +65,23 @@ class AppBase; | |||||||||||||||||||
//------------------------------------------------------------------------------------------------- | ||||||||||||||||||||
/// @ingroup Engine | ||||||||||||||||||||
/// | ||||||||||||||||||||
/// @brief | ||||||||||||||||||||
/// @brief This class implements the keyboard event listener. | ||||||||||||||||||||
//------------------------------------------------------------------------------------------------- | ||||||||||||||||||||
class OSRE_EXPORT KeyboardEventListener : public Platform::OSEventListener { | ||||||||||||||||||||
public: | ||||||||||||||||||||
/// @brief The class constructor. | ||||||||||||||||||||
KeyboardEventListener() : | ||||||||||||||||||||
OSEventListener("App/KeyboardEventListener"), | ||||||||||||||||||||
mLast(Platform::KEY_UNKNOWN) { | ||||||||||||||||||||
clearKeyMap(); | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
~KeyboardEventListener() override { | ||||||||||||||||||||
// empty | ||||||||||||||||||||
} | ||||||||||||||||||||
/// @brief The class destructor. | ||||||||||||||||||||
~KeyboardEventListener() override = default; | ||||||||||||||||||||
|
||||||||||||||||||||
/// @brief The event handler. | ||||||||||||||||||||
/// @param osEvent The os-specific event. | ||||||||||||||||||||
/// @param data The event-related data. | ||||||||||||||||||||
void onOSEvent(const Common::Event &osEvent, const Common::EventData *data) override { | ||||||||||||||||||||
auto keyData = (Platform::KeyboardButtonEventData *)data; | ||||||||||||||||||||
if (osEvent == Platform::KeyboardButtonDownEvent) { | ||||||||||||||||||||
|
@@ -87,14 +93,20 @@ class OSRE_EXPORT KeyboardEventListener : public Platform::OSEventListener { | |||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
/// @brief Returns true, when the key is pressed | ||||||||||||||||||||
/// @param key The key to look for | ||||||||||||||||||||
/// @return true for is pressed. | ||||||||||||||||||||
bool isKeyPressed(Platform::Key key) const { | ||||||||||||||||||||
return mKeymap[key] == 1; | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
99
to
101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add bounds checking to prevent buffer overflow. The direct array access in Add bounds checking: bool isKeyPressed(Platform::Key key) const {
+ if (static_cast<size_t>(key) >= Platform::KEY_LAST) {
+ return false;
+ }
return mKeymap[key] == 1;
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
/// @brief Will return the latest pressed key. | ||||||||||||||||||||
/// @return The latest pressed key. | ||||||||||||||||||||
Platform::Key getLastKey() const { | ||||||||||||||||||||
return mLast; | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+103
to
107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add bounds checking for getLastKey method. The Platform::Key getLastKey() const {
+ if (mLast >= Platform::KEY_LAST) {
+ return Platform::KEY_UNKNOWN;
+ }
return mLast;
}
|
||||||||||||||||||||
|
||||||||||||||||||||
/// @brief Clearn the map. | ||||||||||||||||||||
void clearKeyMap() { | ||||||||||||||||||||
::memset(mKeymap, 0, sizeof(char) * Platform::KEY_LAST); | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+96
to
112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance type safety in keyboard state management. Consider these improvements for better type safety and maintainability:
class OSRE_EXPORT KeyboardEventListener : public Platform::OSEventListener {
private:
Platform::Key mLast;
- char mKeymap[Platform::KEY_LAST];
+ std::array<bool, Platform::KEY_LAST> mKeymap;
public:
KeyboardEventListener() :
OSEventListener("App/KeyboardEventListener"),
- mLast(Platform::KEY_UNKNOWN) {
- clearKeyMap();
+ mLast(Platform::KEY_UNKNOWN),
+ mKeymap{} {
}
void clearKeyMap() {
- ::memset(mKeymap, 0, sizeof(char) * Platform::KEY_LAST);
+ mKeymap.fill(false);
}
bool isKeyPressed(Platform::Key key) const {
return mKeymap[key];
}
|
||||||||||||||||||||
|
@@ -250,6 +262,9 @@ class OSRE_EXPORT AppBase { | |||||||||||||||||||
/// @return The keyboard listener. | ||||||||||||||||||||
virtual KeyboardEventListener *getKeyboardEventListener() const; | ||||||||||||||||||||
|
||||||||||||||||||||
/// @brief | ||||||||||||||||||||
virtual RenderBackend::CanvasRenderer *getCanvasRenderer() const; | ||||||||||||||||||||
|
||||||||||||||||||||
Comment on lines
+266
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolve type mismatch between member variable and getter return type. There's a type mismatch:
This could lead to type conversion issues or undefined behavior if not handled correctly. Either:
- RenderBackend::IRenderPath *mCanvasRenderer;
+ RenderBackend::CanvasRenderer *mCanvasRenderer;
- virtual RenderBackend::CanvasRenderer *getCanvasRenderer() const;
+ virtual RenderBackend::IRenderPath *getCanvasRenderer() const; Please clarify the intended type relationship between these components. Also applies to: 305-305 |
||||||||||||||||||||
protected: | ||||||||||||||||||||
/// @brief The onCreate callback, override this for your own creation stuff. | ||||||||||||||||||||
/// @return true if successful, false if not. | ||||||||||||||||||||
|
@@ -292,6 +307,7 @@ class OSRE_EXPORT AppBase { | |||||||||||||||||||
Common::Ids *mIds; | ||||||||||||||||||||
StageMode mStageMode; | ||||||||||||||||||||
bool mShutdownRequested; | ||||||||||||||||||||
RenderBackend::IRenderPath *mCanvasRenderer; | ||||||||||||||||||||
}; | ||||||||||||||||||||
|
||||||||||||||||||||
inline Stage *AppBase::getStage() const { | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Consider removing redundant Google Test package.
The project uses GTest with modern CMake configuration (
find_package(GTest CONFIG REQUIRED)
) and links against the namespaced targets (GTest::gtest
andGTest::gtest_main
). In Ubuntu,libgtest-dev
is sufficient for this setup, making the additionalgoogletest
package redundant.🔗 Analysis chain
Consider removing redundant Google Test package.
The line installs both
libgtest-dev
andgoogletest
packages, which appear to be redundant as they both provide Google Test framework. This could potentially lead to version conflicts.Consider using only one package:
Let's verify the Google Test setup and package information:
Let me verify the CMake configuration to understand how GTest is being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 295
Script:
Length of output: 4686