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

Prevent integral duplicates (#1038) #1047

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Prevent integral duplicates (#1038) #1047

merged 1 commit into from
Mar 3, 2025

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Feb 28, 2025

Also hide sell button for integral

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Fixes #1038

Hide sell button for integral
@vincele
Copy link
Contributor

vincele commented Feb 28, 2025

Just curious, does that "integral" thing means "integrated", as in part of the ship design itself, fixed and cannot be removed ?

Another question, why does RecomputeUnitUpgrades() does its thing in 3 (almost identical) passes over the unit's cargo items ? Is there some ordering required ?

@royfalk
Copy link
Contributor Author

royfalk commented Feb 28, 2025

Just curious, does that "integral" thing means "integrated", as in part of the ship design itself, fixed and cannot be removed ?

I actually checked before.
Def integral: essential to completeness
Def integrated: with two or more things combined in[order to become more effective or consisting of different groups of people who mix, live, or work well together.

I was looking for a word that would indicate it cannot be removed. Integral fit better.

Another question, why does RecomputeUnitUpgrades() does its thing in 3 (almost identical) passes over the unit's cargo items ? Is there some ordering required ?

Possibly. We are in the process of refactoring legacy code and the original devs are not around to ask. I did see that it seemed redundant somehow but the whole flow is destined for removal soon anyhow.

Copy link

@kheckwrecker kheckwrecker left a comment

Choose a reason for hiding this comment

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

This PR works perfectly. No longer able to sell Integral components and I am able to sell other components and purchase upgrades. This PR is good with me for merging.

@@ -3161,6 +3161,13 @@ bool Unit::UpAndDownGrade(const Unit *up,
bool Unit::ReduceToTemplate() {
vector<Cargo> savedCargo;
savedCargo.swap(cargo);

// Remove integral components, because they become duplicates
savedCargo.erase(std::remove_if(savedCargo.begin(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been carefully tested? It looks like it might be correct, but historically, we have had some issues with deleting items from collections. So better safe than sorry, it seems to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debugged this and placed breakpoints to assure myself it does exactly what I think it does.

In case you're wondering, I went through several iterations of touching the code and realizing that it doesn't solve the problem.

I'm fairly confident of this solution. However, it should go away when we finish migrating to libcomponent.

@vincele
Copy link
Contributor

vincele commented Mar 1, 2025

@royfalk thanks for the explanations

@royfalk royfalk merged commit 51ca151 into master Mar 3, 2025
21 checks passed
@royfalk royfalk deleted the task_several_bugs branch March 3, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants