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

Items not being removed correctly from collections #718

Open
stephengtuggy opened this issue Jul 24, 2022 · 8 comments
Open

Items not being removed correctly from collections #718

stephengtuggy opened this issue Jul 24, 2022 · 8 comments
Assignees
Labels
Milestone

Comments

@stephengtuggy
Copy link
Contributor

In many places in the code, items -- especially owning raw pointers -- are being removed from collections (vectors, etc.) incorrectly. See https://www.fluentcpp.com/2018/09/18/how-to-remove-pointers-from-a-vector-in-cpp/ for more about this type of problem and how to fix it.

@stephengtuggy stephengtuggy self-assigned this Jul 24, 2022
@stephengtuggy
Copy link
Contributor Author

#685 is probably caused by an instance of this type of problem.

@BenjamenMeyer BenjamenMeyer modified the milestones: Triage, 0.9.x Jul 24, 2022
@stephengtuggy stephengtuggy mentioned this issue Aug 2, 2022
1 task
@BenjamenMeyer
Copy link
Member

Let's fix what we can for 0.9.x but I'm okay deferring this as it'll probably be a longer term refactor of the code to fully solve this.

@stephengtuggy
Copy link
Contributor Author

Let's fix what we can for 0.9.x but I'm okay deferring this as it'll probably be a longer term refactor of the code to fully solve this.

Okay. I hate to leave any of these bugs for another major release. But you're probably right that a longer-term refactor will be required to fix many of them.

@stephengtuggy
Copy link
Contributor Author

We do have a couple of things in place now to help us fix many of these issues. The first is the owner<...> construct (in owner.h), which designates an owning raw pointer. We can start using this in many places in the code to help us understand better what the code is supposed to do in that context. However, the owner<...> construct itself doesn't functionally change the structure of the code at all. This should make it easier to adopt.

The second item is the remove_all_references_to template function in vega_collection_utils.h. We might need a few more flavors of it -- or one, more generalized flavor -- for collection types other than vector. Even as it is, though, there should be a lot of places in the code where we can use this to make deleting pointers from vectors more consistent -- and more correct.

With all that said, I understand that there are practical limits to what we can accomplish in the 0.9.x time-frame. So we can leave this ticket in the 'Deferred' column, as far as I'm concerned.

(What is our approximate target date for releasing 0.9.0?)

@BenjamenMeyer
Copy link
Member

With all that said, I understand that there are practical limits to what we can accomplish in the 0.9.x time-frame. So we can leave this ticket in the 'Deferred' column, as far as I'm concerned.

Thanks! it all looks like excellent work.

(What is our approximate target date for releasing 0.9.0?)

@stephengtuggy Basically the major thing holding up the 0.9.0 release is getting a GitHub Action that can upload the artifacts for the release for Windows and Mac. Aside from that its the little bugs that you and @royfalk want to knock out in the mean-time which IIRC are dwindling down.

@stephengtuggy
Copy link
Contributor Author

With all that said, I understand that there are practical limits to what we can accomplish in the 0.9.x time-frame. So we can leave this ticket in the 'Deferred' column, as far as I'm concerned.

Thanks! it all looks like excellent work.

Thank you very much.

(What is our approximate target date for releasing 0.9.0?)

@stephengtuggy Basically the major thing holding up the 0.9.0 release is getting a GitHub Action that can upload the artifacts for the release for Windows and Mac. Aside from that its the little bugs that you and @royfalk want to knock out in the mean-time which IIRC are dwindling down.

Yes, there are fewer and fewer bugs remaining on the 0.9.x project board. However, finishing the GameConfig refactor -- for one -- will probably take some time. And that's one task I would really like to finish before we release 0.9.0, because without it, the game is not functioning as intended. Some of the settings it loads (or doesn't load) are wrong.

Perhaps most noticeable is the lack of animosity between factions. They all start out neutral toward one another now. That's definitely not how it's supposed to be! 😆

@BenjamenMeyer
Copy link
Member

However, finishing the GameConfig refactor -- for one -- will probably take some time. And that's one task I would really like to finish before we release 0.9.0, because without it, the game is not functioning as intended. Some of the settings it loads (or doesn't load) are wrong.

If they're mostly things that have been that way I wouldn't fret about it much. If it's new, I'd agree it should be fixed faster. We can always fix in a 0.9.1 release too; so we can make some improvements along the way to 0.10.x; nothing is saying we can't do intermediate releases - in fact, we probably should be especially if we're going to have these longer cycles.

@stephengtuggy
Copy link
Contributor Author

I think the setting that controls starting relationships between factions may actually be in the audio settings namespace. I forget exactly what the specific setting is called. If I find it again, I will let you know.

Note also that while I started a wiki page that aspired to be a full list of settings, it is still incomplete. Unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants