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

Bobisback Import Changes #958

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

Conversation

Bobisback
Copy link

@Bobisback Bobisback commented Nov 24, 2023

Description

  • Added in a new button on the import screen that allows with one click all demanded goods to be imported into the city.
  • Added in a new button on the import screen that allows with one click all goods needed for construction to be imported into the city.
  • Added all relevant defines to the global defines file for semi convenient customization.
  • Included a small bug fix in the auto export code that was removing food from imports and exports.

Known Impacted Areas of Code

This should only effect the import/export popup. This also changes some code related to the auto trader code related to some small bugs I found.

Additional Notes

  • We have to add import limits on everything so it plays nice with the auto export feature.
  • I did fix a bug in the auto export that was removing food as a import or export no matter what.
  • This code is not great but please keep in mind I copied to be the same as the code that is already there. This way when it is refactored it can all be done together. This should keep things cleaner for the future.
  • I recommend who ever merges this PR squashes my commits, since I do have a bunch of commits in there.

Details for Code Reviewer Running Code

Steps to test for importing demanded goods

  1. Run game
  2. Enter a city that has demanded goods
  3. Optionally reset imports for the city via the imports popup
  4. Use the import all demanded goods button
  5. You should observe that all goods that are demanded are set to a 5 turns worth of goods for the maintain level and 8 turns worth of goods for the import limit level.
  6. Done! Report any issues or questions here and has much success!!

Steps to test for importing Construction goods

  1. Run game
  2. Enter any city
  3. Optionally reset imports for the city via the imports popup
  4. Use the import construction goods button
  5. You should observe that construction goods are set to maintain and limit values that are in the global defines
    (100 for lumber, clay, and stone, 150 for tools) Again we have to set limit values otherwise it will break auto export.
  6. Done! Report any issues or questions here and has much success!!

Steps to test for auto export bug

  1. Run game
  2. Enter any city
  3. Add a food import or export
  4. Use the auto export all goods button
  5. You should observe that the food import/export should be unchanged
  6. Done! Report any issues or questions here and has much success!!

Author's Checklist

  • Did you run the code and verify that it works?
  • Is the code refined/iterated/beautiful?
  • Does your stuff deallocate well (ie was things cleaned up after being used)?
  • Are warnings in classes you are using cleaned up or suppressed?
  • Did you think about networking and not creating desyncs?
  • Privacy: Do you prevent any sensitive user information from being exposed?
  • Security: Have you considered and addressed any potential security concerns?
  • Did you leave relevant but not overstated comments in your code?

Other Reviewers

  • Did you run the code? Does it work? Add screenshots/videos as needed for failed items.
  • Is the code as abstracted, composable, and isolated as possible?
  • Privacy: Is any user information being exposed that shouldn’t be?
  • Security: Does this have any security concerns?
  • Are your comments to the author positive?

@MrZorG33
Copy link
Collaborator

MrZorG33 commented Nov 24, 2023

I did fix a bug in the auto export that was removing food as a import or export no matter what.

this is not a bug. this was done on purpose because food is required for the growth of the colony. if the player needs, he can manually set the export/import of food.

upd I am against this correction.

@Bobisback
Copy link
Author

Bobisback commented Nov 24, 2023

I did fix a bug in the auto export that was removing food as a import or export no matter what.

this is not a bug. this was done on purpose because food is required for the growth of the colony. if the player needs, he can manually set the export/import of food.

upd I am against this correction.

Hey MrZorG33,

I talked with Nightinggale and Johan Buret / Barthoze on this subject in discord. The discussion starts here if you are interested in reading the details.

The TLDR is this:
So I agree with what you said, and this is why I did this change. If there is food imported or exported for a city then the auto export button should completely ignore food. But it was not, for some reason if food was set to import or export then when you clicked auto export button it was removing food from both imports and exports (it was also deleting import limits and maintain amounts). Hence this bug fix. Originally I was going to put a check in there to ignore food but based on feedback from Nightinggale and Johan Buret / Barthoze they both said that the code that was causing the issues was not needed and could be removed instead.

Hopefully this clarifies some things!

Thanks,
Bobisback

@MrZorG33
Copy link
Collaborator

Thanks @Bobisback, let's try to find out. (By the way, I can't open your link - error 404)

Yield export is initially configured in CIV4YieldInfos. @Nightinggale, @johan-buret, why are there other export settings that conflict with the original settings?

@Bobisback
Copy link
Author

Thanks @Bobisback, let's try to find out. (By the way, I can't open your link - error 404)

Yield export is initially configured in CIV4YieldInfos. @Nightinggale, @johan-buret, why are there other export settings that conflict with the original settings?

I had the link incorrectly set. try now it works! I tested it.

@MrZorG33
Copy link
Collaborator

MrZorG33 commented Nov 24, 2023

I read your correspondence.
if necessary for the code to work correctly, try setting the < bIsExportYield > parameter to 0 for Food.

but once again, food should not be automatically added to the export list, since it is the only important yield for the growth of the colony. its export must be configured manually.

@Bobisback
Copy link
Author

Bobisback commented Nov 24, 2023

I read your correspondence. if necessary for the code to work correctly, try setting the < bIsExportYield > parameter to 0 for Food.

but once again, food should not be automatically added to the export list, since it is the only important yield for the growth of the colony. its export must be configured manually.

I think you are misunderstanding what the bug is. The bug is that this function that sets all the auto exports for everything is removing Food as a import and export. That is the bug. Aka I go into a city. I set food as a import or export manually. I click auto export everything. Food is then removed as an import/export for the city by this function. I fixed it so now when you click auto export it does not remove food as a import or export in the city. Aka if the user sets food as a import or export, my bug fix makes it so the auto export function DOES NOT remove food as a import or export it just completely ignores food altogether now. Just to be super clear, if the user sets food up manually this function in the current code base will delete that setup.

P.S we discussed setting bIsExportYield to 0 on food but based on this discussion they determined it would break other things. Please see the discord convo for details.

@Bobisback
Copy link
Author

I read your correspondence. if necessary for the code to work correctly, try setting the < bIsExportYield > parameter to 0 for Food.

but once again, food should not be automatically added to the export list, since it is the only important yield for the growth of the colony. its export must be configured manually.

And to be super super clear, with my change the auto export button DOES NOT set food up for auto export. In fact it ignores food completely.

@MrZorG33
Copy link
Collaborator

Apparently due to incorrect translation, I do not always correctly understand what some people write. although this also works in reverse)
well, if auto-export of Food is not turned on, that's good.

P.S we discussed setting bIsExportYield to 0 on food but based on this discussion they determined it would break other things. Please see the discord convo for details.

Well, if they think so, ok.

@XSamatan
Copy link

@raystuttgart @Nightinggale

Will this be included in a new version?

I'm going to update my local copy to the newest revision next week if feedback about this feature is needed.

Regards
XSamatan

@raystuttgart
Copy link
Collaborator

raystuttgart commented Jan 22, 2024

@XSamatan
I have not been following the discussion and I am currently not modding.
The people that are currently active need to decide if they want to commit and also test it ingame.

Some active team member needs to take responsibility for it.

@Bobisback
Copy link
Author

Did anything ever become of this? Should we give up on this and close the PR? or does it just need more testing? I tested it pretty extensively across two large games. But that does not mean there is not issues, plus we would need to update it to current code base and test again.

@XSamatan
Copy link

I have three games until independence under my belt (and countless mid-game restarts) and never encountered any issues with this pr. All develop version from mid January.

Settings for import are working as described.

I'd like to see this accepted into the mod, would be a nice QOL feature to keep the domestic market interesting in large empires (and prevent it to stay the click-festival it is right now)

Regards
XSamatan

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.

4 participants