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

Mostly memory management #2

Merged
merged 10 commits into from Mar 8, 2021
Merged

Mostly memory management #2

merged 10 commits into from Mar 8, 2021

Conversation

Guekka
Copy link
Contributor

@Guekka Guekka commented Mar 8, 2021

For review, I suggest looking at each commit individually.

For 05db039 (memory management), I suggest you do not look at the headers. It's only noise, the real changes are in the source files

Going to test the changes soon

@ousnius
Copy link
Owner

ousnius commented Mar 8, 2021

@Guekka Please remove "Utils.hpp" and put the changes into "NifUtil.hpp" instead. No need for two util headers.

Additionally, don't include "Utils.hpp" in any other header files, we don't want to force the templates on anyone using nifly. If necessary, move GetBlockID to a source file and split the clone stuff into its own header so that you can include it in other headers if necessary.

- Split Utils.hpp into NifUtil.hpp and CloneInherit.hpp
- Avoid include NifUtil in headers
@Guekka
Copy link
Contributor Author

Guekka commented Mar 8, 2021

@ousnius Is it good like that?

Comment on lines +1410 to +1411
hdr.ReplaceBlock(GetBlockID(shape), std::unique_ptr<NiObject>(std::move(bsOptShape)));
UpdateSkinPartitions(bsOptShape.get());
Copy link
Owner

Choose a reason for hiding this comment

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

@Guekka Commit 3e2e36b is breaking optimization because bsOptShape ptr is moved away before calling UpdateSkinPartitions.

@ousnius ousnius merged commit b03f2f2 into ousnius:main Mar 8, 2021
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.

None yet

2 participants