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

chore: Update render component and delete unused code #1429

Merged
merged 14 commits into from
Jan 31, 2024

Conversation

jadebenn
Copy link
Collaborator

A few minor changes I'm exporting from my entity/component refactor branch. I've also deleted the explicit destructors here, as they're (now) unnecessary and can prevent the implicit generation of move constructors.

@jadebenn jadebenn changed the title Update a few components to use smart pointers for memory management chore: Update a few components to use smart pointers for memory management Jan 20, 2024
Copy link
Collaborator

@EmosewaMC EmosewaMC left a comment

Choose a reason for hiding this comment

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

I wouldn't replace the pointer with a unique ptr, instead consider just removing the ptr altogether and storing the struct as a non ptr in a vector instead.

@jadebenn
Copy link
Collaborator Author

In the remaining case I do think it makes sense to use unique ptrs. There needs to be a "null" case, I think.

@jadebenn jadebenn changed the title chore: Update a few components to use smart pointers for memory management chore: Update render component to use smart pointers for memory management Jan 20, 2024
@jadebenn jadebenn changed the title chore: Update render component to use smart pointers for memory management chore: Update render component to use smart pointers for memory management and delete unused code Jan 20, 2024
@jadebenn jadebenn requested a review from EmosewaMC January 20, 2024 06:21
@EmosewaMC
Copy link
Collaborator

EmosewaMC commented Jan 20, 2024

In the remaining case I do think it makes sense to use unique ptrs. There needs to be a "null" case, I think.

Where is this null check done? Can there not be an alternative check if it exists? I do not believe Effect needs to be a ptr as the only case where we return one we always assume its non-null, therefore this pointer can be safely removed.

@jadebenn
Copy link
Collaborator Author

jadebenn commented Jan 20, 2024

There is a null check in the serialization. I'm not 100% sure of its purpose, but I was reluctant to touch it.

@EmosewaMC
Copy link
Collaborator

There is a null check in the serialization. I'm not 100% sure of its purpose, but I was reluctant to touch it.

That is simply a good practice when accessing pointers. The member does not need to be a pointer

@EmosewaMC
Copy link
Collaborator

There are cases when you don't need to check (like when there is a strong guarantee the pointer will be valid, like the parent of a component), but for a container like this someone may mindlessly delete the ptr but not remove it from the container. In this context it was just added as a backup.

@jadebenn jadebenn changed the title chore: Update render component to use smart pointers for memory management and delete unused code chore: Update render component and delete unused code Jan 21, 2024
Copy link
Collaborator

@EmosewaMC EmosewaMC left a comment

Choose a reason for hiding this comment

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

one final question

@jadebenn
Copy link
Collaborator Author

I still haven't gotten around to checking the specific red blocks case but I didn't notice anything out of the ordinary running around VE and AG when I last tested. I'll try and confirm your specific example tonight.

@EmosewaMC
Copy link
Collaborator

Ah, some context that may be helpful is red blocks is the only place in the whole game where this component is serialized, hence this test case

@jadebenn
Copy link
Collaborator Author

Tried it in-game. I don't think this is normal... is it?
image

@jadebenn
Copy link
Collaborator Author

Oddly, it's only the speaker that seems to have issues

@jadebenn jadebenn marked this pull request as draft January 30, 2024 03:54
@jadebenn
Copy link
Collaborator Author

Marking as draft until this is solved.

@EmosewaMC
Copy link
Collaborator

Tried it in-game. I don't think this is normal... is it? image

Normal and live accurate behavior. The live game used the wrong fx for that speaker (crux prime meteor instead of the intended one.)

@EmosewaMC
Copy link
Collaborator

Check the client logs for the word unserialize and if you see any let me know.

@jadebenn jadebenn marked this pull request as ready for review January 31, 2024 03:07
@jadebenn
Copy link
Collaborator Author

Checked, no issues in the logs. Should recompile fine if you rerun the workflow.

@jadebenn jadebenn requested a review from EmosewaMC January 31, 2024 03:07
Copy link
Collaborator

@EmosewaMC EmosewaMC left a comment

Choose a reason for hiding this comment

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

Pending ci

@aronwk-aaron aronwk-aaron merged commit b23981e into DarkflameUniverse:main Jan 31, 2024
4 checks passed
@jadebenn jadebenn deleted the UseSmartPointers branch January 31, 2024 15:32
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.

3 participants