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

Standartize metadata for all objects on Adventure Map #7084

Merged
merged 54 commits into from
May 5, 2023

Conversation

ihhub
Copy link
Owner

@ihhub ihhub commented Apr 30, 2023

  • remove cryptic quantity1 and quantity2 values everywhere in the code and replace them with human-readable metadata array
  • avoid hardcoded values for many object conditions like Artifact, Daemon Cave or Shipwreck
  • fix AI pathfinding when Spell Scroll was considered as an artifact which requires to defeat an army
  • fix Shipwreck Survivor bonuses. It always contained artifact of level 1
  • add many checks in the code to prevent calling functions for unsupported objects
  • add specialized functions to get information about objects. We have to avoid generic functions

relates to #6845

ATTENTION
This pull request changes almost every object interaction on Adventure Map in relation to conditions and resources. Also I added a conversion code from old save format to a new one. As a result, it is important to test all objects for new maps as well as objects on maps loaded from old saves.

The best strategy for testing by developers is to enable debug mode and see metadata values on objects. Now it is easily readable.

@ihhub ihhub added bug Something doesn't work improvement New feature, request or improvement logic Things related to game logic editor Map editor related stuff labels Apr 30, 2023
@ihhub ihhub added this to the 1.0.4 milestone Apr 30, 2023
@ihhub ihhub self-assigned this Apr 30, 2023
@ihhub ihhub marked this pull request as draft April 30, 2023 04:35
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

@LeHerosInconnu
Copy link

Hello @ihhub,

Hi @LeHerosInconnu, could you please help to test changes in this pull request? We need your comprehensive testing skills for this pull request.

Are there any other changes planned before I start the test?

@ihhub
Copy link
Owner Author

ihhub commented May 3, 2023

Hello @ihhub,

Hi @LeHerosInconnu, could you please help to test changes in this pull request? We need your comprehensive testing skills for this pull request.

Are there any other changes planned before I start the test?

Nope. I've recently fixed some issues found by @Branikolog and reported through Discord. As of now only bug fixing. You can download the newer version once the compilation is done.

@oleg-derevenetz
Copy link
Collaborator

Hi @ihhub is this fine that when Tree of Knowledge has no resource requirement, there is "unknown resource" warning printed in the Funds constructor? It doesn't interfere with the game, but it looks weird and litters the log with messages about what is not really a problem.

@ihhub
Copy link
Owner Author

ihhub commented May 3, 2023

Tree of Knowledge

Hi @oleg-derevenetz , actually it is wrong. This message is added to avoid bad written code. I will have a look at it. Thank you for spotting it!

@ihhub
Copy link
Owner Author

ihhub commented May 3, 2023

@oleg-derevenetz , fixed!

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented May 3, 2023

@ihhub

fixed!

What do you think about the following fixing method instead: allow to pass the Resource::UNKNOWN value to the Funds constructor (add the special switch case with just break and maybe also a check that count is zero as well and log a warning if it is not, because there cannot be non-zero amount of unknown resource)? Or this is undesirable?

@ihhub
Copy link
Owner Author

ihhub commented May 3, 2023

What do you think about the following fixing method instead: allow to pass the Resource::UNKNOWN value to the Funds constructor (add the special switch case with just break and maybe also a check that count is zero as well and log a warning if it is not, because there cannot be non-zero amount of unknown resource)? Or this is undesirable?

I think we should prevent even passing unknown types into the constructor. This might help to find some future issues with wrong code or wrong data read from files. Of course no harm by doing this but in my view we should log such undesired behavior.

@oleg-derevenetz
Copy link
Collaborator

Hi @ihhub there is the same issue in the Maps::getFundsFromTile(): non-pickable objects may contain no funds.

@ihhub
Copy link
Owner Author

ihhub commented May 3, 2023

Hi @ihhub there is the same issue in the Maps::getFundsFromTile(): non-pickable objects may contain no funds.

... I shall add the case with zero unknown resources :(

@LeHerosInconnu
Copy link

Hello @ihhub,

I did a test on a test scenario and did not encounter any problems.
I will do another test playing a standard scenario.

@ihhub
Copy link
Owner Author

ihhub commented May 4, 2023

Thank you @LeHerosInconnu !

Copy link
Collaborator

@Branikolog Branikolog left a comment

Choose a reason for hiding this comment

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

Haven't found more bugs here. Great job, fheroes2 team! :D

Copy link
Collaborator

@zenseii zenseii left a comment

Choose a reason for hiding this comment

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

Hi, @ihhub. I've tested some old saves I had and done a partial read-through of the code. So far I've only found these typos. Please have a look at them when you have time.

@ihhub ihhub requested a review from zenseii May 4, 2023 15:35
Copy link
Collaborator

@zenseii zenseii left a comment

Choose a reason for hiding this comment

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

Hi, @ihhub. I've read through all the changes and found only this typo which will need to be changed overall.

@ihhub ihhub requested a review from zenseii May 5, 2023 00:09
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 17 Code Smells

0.0% 0.0% Coverage
5.2% 5.2% Duplication

Copy link
Collaborator

@Districh-ru Districh-ru left a comment

Choose a reason for hiding this comment

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

I played fully one new standard game and for a couple of week a campaign scenario and a standard game from an old save file. I did not notice any bugs.
Well done!

@ihhub ihhub merged commit 702c7d1 into master May 5, 2023
@ihhub ihhub deleted the quantity_eradication branch May 5, 2023 13:11
@LeHerosInconnu
Copy link

Hello @ihhub,

Hello @ihhub,

I did a test on a test scenario and did not encounter any problems.
I will do another test playing a standard scenario.

I finished a standard scenario started with the PR version and continued with the merged version without any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something doesn't work editor Map editor related stuff high priority Very critical change needed immediately improvement New feature, request or improvement logic Things related to game logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants