Fix for DvScene Editor#35
Merged
angryzor merged 16 commits intoHE2-SDK:mainfrom Oct 27, 2025
Merged
Conversation
|
angryzor
requested changes
Sep 18, 2025
Collaborator
There was a problem hiding this comment.
General comments:
- Please fix the memory leak. I will not greenlight a merge unless the code is memory leak free.
- I think you're making the
Previewer/PreviewCameraseparation too complex. I'd suggest to do away with theGetTextureID()function and instead just have aRender()function onPreviewerso that it's a proper ImGui component, and then instead ofPreviewCamerajust have anInteractiblePreviewclass that creates aPrevieweritself and in its ownRender()function calls thePreviewersRenderfunction and also renders the camera controls. Now you have thisSetPreviewer()thing and then make assumptions about the environment in whichRenderis called (it assumes it can take all available space). I understand what you are trying to do by decoupling but I feel like it's not gaining you much and it's more useful for consumers to just be able to dointeractiblePreview.Render()and not have to worry about texture ids and connecting the camera manipulation separately. - I'd also just rename
PreviewertoPreview. - It's ok since the CEMT editor is still in development, but please use human friendly labels for
Editor()calls. Just a single sentence, in normal sentence capitalization (don't capitalize each word). - Please don't put opening braces on a new line. I know there's some instances of that in the codebase because VS keeps injecting them, but I'm trying to move away from that style, and they will be removed when the linter is added.
- Please use initializer syntax more. You don't have to change this in this code, but just in the future.
| edited |= Editor("startDelay", emitter.startDelay); | ||
| edited |= Editor("fadeSpeed", emitter.fadeSpeed); | ||
| edited |= Editor("lifeEndTime", emitter.lifeEndTime); | ||
| edited |= Editor("accelarationMultiplier", emitter.accelarationMultiplier); |
| const size_t resNameLen = strlen(resName); | ||
|
|
||
| const size_t nameLen = resNameLen + nameTemplateLen + 1; | ||
| char* nameRaw = static_cast<char*>(pAllocator->Alloc(nameLen, 4)); |
Collaborator
There was a problem hiding this comment.
please don't directly call Alloc
Collaborator
There was a problem hiding this comment.
you're also leaking memory
Collaborator
There was a problem hiding this comment.
(Just use a VariableString for the name in the class)
Contributor
Author
There was a problem hiding this comment.
would use variablestring, but i have no idea on how to use a dynamic sized snprintf with it. for now ill just replace static_cast<char*>(pAllocator->Alloc(nameLen, 4)); with new (GetAllocator()) char[nameLen];
Contributor
Author
|
I have decided to move the viewport to a separate branch, so that some fixes and other content can be merged and released. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.