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

planet size calculation #391 fixes #507

Merged
merged 7 commits into from
Dec 24, 2024

Conversation

yazilimmz
Copy link
Contributor

I made changes based on this page. Adjustments were made according to the first planet.

Copy link
Owner

@lanedirt lanedirt left a comment

Choose a reason for hiding this comment

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

Hi @yazilimmz,

Thanks for your contribution and for looking into implementing the planet size calculations!

Overall the changes to the field calculation looks good to me. I reviewed the changes and did add two comments, see below. Could you take a look at these?

If you have any questions feel free to let me know. Thanks again for your time in helping out, much appreciated! 🚀 😃

app/Factories/PlanetServiceFactory.php Outdated Show resolved Hide resolved
app/Factories/PlanetServiceFactory.php Outdated Show resolved Hide resolved
@yazilimmz yazilimmz requested a review from lanedirt December 24, 2024 08:14
@yazilimmz
Copy link
Contributor Author

Hello,
Thank you for starting and leading this project; we truly appreciate your efforts.
You're absolutely right—I hadn't realized that the percentage pointed there. I've made adjustments accordingly.

Using this link:
https://proxyforgame.com/en/ogame/calc/production.php

I noticed that it corresponds to the base production, so I updated it based on that.
If you could create an issue and assign it to me for calculating energy and deuterium based on the planet's temperature, I’d be happy to look into it as well.

@@ -1462,6 +1520,7 @@ public function getPlanetBasicIncome(): Resources
*/
private function updateResourceProductionStatsInner(Resources $production_total, int|float $energy_production_total, int|float $energy_consumption_total, bool $save_planet = true): void
{

Copy link
Owner

Choose a reason for hiding this comment

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

It looks like nothing changed in this method updateResourceProductionStatsInner() except adding and removing a newline, can you revert the change on line 1523 and line 1482?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like nothing changed in this method updateResourceProductionStatsInner() except adding and removing a newline, can you revert the change on line 1523 and line 1482?

I have updated the code again for your reference. Since the production calculations are added to the base production, you can see it in PlanetService::getPlanetBasicIncome().

@yazilimmz yazilimmz requested a review from lanedirt December 24, 2024 09:10
@lanedirt
Copy link
Owner

Hi @yazilimmz,

Thanks for the adjustments, functionally it looks good to me now. I noticed that some tests were failing, but as I was typing this I saw your new commit come in and now it looks like its working properly, nice 😄! Will merge this PR now. Welcome as an official contributor to the OGameX project 🚀 !

For the other changes you mentioned earlier regarding energy and deuterium output an issue already exists, see:

If you have the time to look at this it would be greatly appreciated :-)

@lanedirt lanedirt merged commit 1d05dbc into lanedirt:main Dec 24, 2024
5 checks passed
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.

2 participants