Fix wrong ressources stats behavior & Change Ressource Setting view

[veikoon]

Description

Please include a summary of the changes made to OGameX and their purpose. Clearly describe what this PR does and why it is necessary.

Type of Change:

• Bug fix
• Feature enhancement

Related Issues

Fixes #375

Additional Information

• Remove energy from Resources sum() function as the sum function is used to get the ammount of resource product by a Building (therefor - the energy used should not be part of the calcul)
• Add a verification on TechTreeController to check if the Building is an energy producer to get the energy production instead of sum() --> As the Resource sum function does not take energy anymore
• Fix Resource getFormatted() functions as in ressource blade a parametter were given (nb of hour for day and week) but were not implemented. --> The numbers of materials product are now formatted
• Change the Energy collumn in the Ressource setting to transform the view as in the real Ogame (energy_used / energy_max) instead of the addition of energy diff over days and week as cumulation of energy doesn't really make sense because energy diff is lost.
• Fix the calculation of Building Techinfo (the building percentage is define on a 10 base instead of a 100 one)
• Fix the Header Current production by removing a updateResourceProductionStatsInner (the Current production were actualy base income + 2 * Building income because of the double call) --> I do read the following explanation :
  // 1. First time in order to ensure the energy production is up-to-date.
  // 2. Second time for the mine production to be updated according to the up-to-date (actual) energy production
  But I didn't make sense in my mind, and removing one call and testing just patch the issue.

--> 515 + 36.9 = 551.9 ~= 551

--> The stats are corresponding to the actual production

--> The formatting is correct, you can view the exact number with your mouse & the energy is the live consumption

[veikoon]

Mmh It seems that the double update for planet needs to be done for the createAndSetPlanetModel from Unit Tests.

[lanedirt]

Hi @veikoon,

Thanks for putting the time in to investigate the resource production issue and opening up a PR, nice work!

I have tested the code locally and reviewed the changes. I have a few comments. Could you take a look at these? If you have any thoughts, feel free to share.

I do need to say: kudos for the depth of your debugging! Seems like you did find what is most likely the root cause in that duplicate resource production calculation. So good job on the deep-dive! 😄 With this version all calculations on my local testing environment now indeed seem to match so I'm very happy 👍 !

[lanedirt]

Great catch on identifying the duplicate method call! After revisiting this, it’s clear that this is one of the main causes of the discrepancy in resource production. The method itself directly appends the calculated production values to the planet object. So when called twice, it will append these values again, which isn’t correct.

The reason the method is called twice was to respect the correct order of operations: first, energy production/consumption needs to be calculated, and then those totals are used to calculate mine production, as they rely on accurate energy data. However now that you have removed the second call, in regular gameplay it would now take two separate requests for everything to be properly calculated. This is why you saw the unit tests failing, as everything is done in a single request there. Adding back the second pass only in the unit tests is not ideal as the tests now have different logic compared to real player gameplay requests, though I understand your reasoning behind it for trying.

It would likely be better to refactor updateResourceProductionStatsInner() further and break it into distinct steps:

1. Calculate energy production/consumption for all objects and set it on the planet model. We can ignore the resource production (metal/crystal/deut) at this stage.
2. Make a second pass that only handles resource production, ignoring energy consumption, so that the two steps don't overwrite each other.

For naming, perhaps:

1. updateEnergyProductionStats()
2. updateResourceProductionStats()

Then, for the third step, the logic for the fusion plant would rely on the output from step 2, so that could be handled as a separate part.

If you have time to look at this further to improve it, feel free to do so. But if you don't I'm also happy to merge your current change as-is as it's already better than the previous implementation. The additional refactoring can then happen in a later PR.

[veikoon]

Hi @lanedirt !

I'm glad you appreciate my humble work 😃
I listened to you and applied the changes you requested.

As you said I split the updateResourceProductionStatsInner() behavior, please review it as it is not quite easy to explain.
Here is the logic :

• Get the energy consumption & production from all building except the Fusion Reactor
• "Save" the energy max & used

If player has at least 1 deuterium:

• Calculate the energy produced by the fusion plant and "save" it
• Calculate the deuterium produced by the deuterium synthesizer (In this case the Fusion Reactor can help the production of more deuterium)

If player don't have any deuterium left:

• Calculate deuterium production (without Fusion Reactor)
• Calculate Fusion reactor production

If the production is still positive :

• Add deuterium & energy production (Without the fusion reactor boosting the Deuterium production)

Else:

• Nothing is produced ? Honestly I don't know the game that much, It make sens to me to not produce energy at all rather than calculate a ratio based on how much deuterium is produced compared to the maximum needed.

Then:

• Calculate the Metal & Crystal production (With the energy boost from Fusion Reactor)

I took profit from a new code session to patch other strange behavior here is the list :

Fusion Reactor :
Add building_percentage to the operation as mentionned in the wiki
Remove resource_production_factor from the deuterium (minus) production of the fusion reactor as it should not impact deuterium consumption
Remove the Fusion Reactor from the getResourceProductionFactor calculation as the fusion reactor should not impact the factor for other buildings
Set the fustion reactor stat in ressource setting view overmarked to be more visible

Set the production_factor to float as the building resource production result is rounded anyway, and the sum of energy consumed by buildings was not equal to the available energy (due to an approximation on the factor)

Please tell me what you're thinking of all these changes, for now I won't push new features on this MR but I will correct things as you desire.

[lanedirt]

Thanks so much for all the effort you've put into this PR, I really appreciate your time and thought that went into these changes. 🙏

That said, I've tried testing the updated logic and while some parts are indeed improved, others seem to have unintended side effects or regressions (e.g. where the existing unit test that is now broken without the added solar plant production change). With multiple core mechanics touched in a single PR, it becomes quite difficult to properly test and validate that everything still works as expected.

Honestly I don't know the game that much, It make sens to me to not produce energy at all rather than calculate a ratio based on how much deuterium is produced compared to the maximum needed.

Totally understandable. These resource production calculations are tricky and often non-obvious, especially if you're not deeply familiar with the official game mechanics. I strongly recommend playing the original game a bit so you can manually compare certain usecases to get a better sense of expected outcomes.

To help us move forward, I actually would like to suggest we revert the changes made to PlanetService::updateResourceProductionStatsInner() back to the original implementation. Then I'll be able to review all other changes in isolation and we can get this PR merged.

Then for the rest of the work to the core resource production calculation, we can try break these improvements into smaller, focused PRs. That way, we can properly review, test, and merge changes incrementally, reducing the risk of breaking other parts of the game.

Let me know what you think! I'm happy to help review follow-up PRs in smaller chunks.

[lanedirt]

Typo: tu consume --> to consume

[lanedirt]

Instead of hardcoding production of metal/crystal/deuterium by looking directly at the buildings metal_mine, crystal_mine (and deuterium_synthesizer in this method above), it would be better to stick to the original implementation where it loops through all possible buildings and calculates the total of all production.

This is important because it's possible that in the future other buildings will be added that can produce metal/crystal/deut, and this method should be generic enough that it automatically picks them up without needing to change the code.

So I advise to look at the original implementation before your changes, and use that as a reference point for how to do the calculation.

Edit: see my general comment above, perhaps better to simply revert the changes to this method and then we can take up these changes in new PR's.

[lanedirt]

These lines should be removed as this interferes with the actual test assertion. The test checks for Fusion plant should produce energy when deuterium production is positive but deuterium storage is 0

Since you're adding positive solar plant energy to the mix, it's not able to test the fusion plant energy output in isolation.