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

memory cleanup #630

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

memory cleanup #630

wants to merge 17 commits into from

Conversation

sandsmark
Copy link
Contributor

clarified some of the object/memory ownership by cleaning up most of the bare pointers.

also some other minor cleanup.

@alexeevdv
Copy link
Contributor

Thank you for your contribution. It is very hard to review pull request with 100+ changed files. Could you please split it to smaller parts?

@JanSimek
Copy link
Contributor

Hey Alex, sorry for being off-topic, but have you considered adding new members to the Falltergeist team? @sandsmark or @adamkewley are obviously experts in C++, so they could help push the project forward (I don't know if they are interested or not).

It is just an idea because some pull requests have been hanging for years without any feedback and it is discouraging new contributors from participating 😞

@alexeevdv
Copy link
Contributor

Being expert in C++ in general doesn't make you expert in software architecture, game development, etc. I'm not expert myself too but i have my own vision how this project should look like. I had negative experience in the past when some contributors had full access to repository and were able to commit what ever they want. It ended up in total mess in code base. Including strange variable namings and architecture decisions.
Sadly i dont have much time for project now but still dont want to add even more mess to existing code base. It requires huge amount of time for refactoring and changing some architecture decisions.
I really apreciate all efforts made to support this project but as in any other open source project if you want your contribution to be adopted fast - make it small. It is really almost impossible to review pull requests with 100+ changed files

@adamkewley
Copy link
Contributor

adamkewley commented Jun 30, 2020

@JanSimek , thank you for your kind words, but I'm by no means an expert in C++ :D, those guys are magical. I've written C++ for a few employers (incl. current), the main thing I've learnt is that different developers have different ways of working on things.

@alexeevdv , completely understand what you're saying. I maintain other projects myself, and one thing that I appreciate is that founders/core maintainers work on completely different timescales from contributors who occasionally PR (e.g. founders look at things in terms of years vs. a weekend or two). Add to this the fact that you (and other PRers, probably) can only work on Falltergeist for a very limited amount of time each month/year and, yeah, I wouldn't want to spend that limited time picking though some driveby-contributor's code either.

I think that, moving forward, the best thing I can work on--when I get time, which is almost never--is to add additional tools to klamath, including adding better parser codes for the various fallout files (FRM, DAT, etc.), and adding add CLI utilities for inspecting things. That will never become a game (they're just basic tools), but it might provide some useful nick-nacks to developers like @alexeevdv , and other game engine developers (e.g. @pingw33n , vault13; @darkf, darkfo). This might be a good way of bridging the chasm between "fallout has weird file formats and requirements" and "lets make a game using that".

EDIT: the significance of bridging that chasm is that it will either mean: a) new game engine projects pop up and build on top of it or b) people start to contribute to the existing projects because they feel more comfortable with the concept.

@sandsmark
Copy link
Contributor Author

Thank you for your contribution. It is very hard to review pull request with 100+ changed files. Could you please split it to smaller parts?

Yes. Some parts are hard to split out (changing the type of things touches a lot of stuff), but I'll try to separate out independent parts of it.

Some of the things depend on eachother so there are some regression from the earlier commits resolved in later commits (e. g. when things are in shared pointers and reference counted it leads to some memory "leaks" because the objects are never explicitly released). I hope that's okay, it should be possible to test separately and verify that the later PRs fixes the regressions.

@sandsmark
Copy link
Contributor Author

Being expert in C++ in general doesn't make you expert in software architecture, game development, etc. I'm not expert myself too but i have my own vision how this project should look like. I had negative experience in the past when some contributors had full access to repository and were able to commit what ever they want.

This is why I don't want access. :-)

I like to think that I'm a bit above average experienced in software architecture and game design, but this is also why I don't want direct access. I have the same (negative) experience, and I'm not comfortable with committing things directly without proper review from the original developers that have the experience (and know what has been tried before and won't work) and know the overall vision/ideas behind how things are structured and designed.

Sometimes when I pick up abandoned projects I don't have the luxury of interacting with the original developers, and it's not a fun experience (e. g. after a major rewrite you suddenly discover why it wasn't done that way in the first place).

@alexeevdv
Copy link
Contributor

alexeevdv commented Jul 1, 2020

Could you please make separate pull request with changes that are not related to game objects? F.e. https://github.com/falltergeist/falltergeist/pull/630/files#diff-e71f2afd81e6bd4da0fbdf02d9e4be55R149

Also please split refactoring\improvement and switching from raw to smart pointers.

This way it is much easier to merge and follow what is changed. Thanks in advance

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

4 participants