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

Fix passabilities of objects on map #3756

Merged
merged 12 commits into from
Jun 30, 2021
Merged

Fix passabilities of objects on map #3756

merged 12 commits into from
Jun 30, 2021

Conversation

ihhub
Copy link
Owner

@ihhub ihhub commented Jun 25, 2021

The original code was based on hacks and it wasn't reliable. The current approach uses an algorithm and a set of conditions to determine passability of a tile.

close #1951
close #3750
close #3745
close #1521
close #496
close #2601
close #2401
close #2364
close #3246

@ihhub ihhub added bug Something doesn't work logic Things related to game logic labels Jun 25, 2021
@ihhub ihhub added this to the 0.9.5 milestone Jun 25, 2021
@ihhub ihhub self-assigned this Jun 25, 2021
@ihhub
Copy link
Owner Author

ihhub commented Jun 25, 2021

Hi @LeHerosInconnu, could you please test these changes as the highest priority? The changes only work on new games. To speed up the work you could consider modifying debug parameter in configuration file to 65535 giving you an extra powerful hero.

I would like to know that we didn't screw gameplay with these changes as the code is re-written from scratch.

@sonarcloud
Copy link

sonarcloud bot commented Jun 25, 2021

Copy link
Collaborator

@oleg-derevenetz oleg-derevenetz 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 Since I'm not very familiar with map objects passability rules, I left a few questions here. Would you mind taking a look?

src/fheroes2/maps/maps_tiles.cpp Outdated Show resolved Hide resolved
src/fheroes2/maps/maps_tiles.cpp Show resolved Hide resolved
Copy link
Collaborator

@idshibanov idshibanov left a comment

Choose a reason for hiding this comment

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

Overall new logic looks good to me, although with such a big PR it's impossible to check every case. Few testing sessions definitely needed.

@ihhub
Copy link
Owner Author

ihhub commented Jun 26, 2021

Hi @oleg-derevenetz , I addressed our comments. Could you please take a look again?

@LeHerosInconnu
Copy link

Hello @ihhub,

Hi @LeHerosInconnu, could you please test these changes as the highest priority? The changes only work on new games. To speed up the work you could consider modifying debug parameter in configuration file to 65535 giving you an extra powerful hero.

I would like to know that we didn't screw gameplay with these changes as the code is re-written from scratch.

It sounds like you are still working on this.
Are there any adjustments that need to be made before the change is in its "final" version?
I wouldn't want to start testing again.
Please tell me when it will be ready for testing.

@ihhub
Copy link
Owner Author

ihhub commented Jun 26, 2021

Hello @ihhub,

Hi @LeHerosInconnu, could you please test these changes as the highest priority? The changes only work on new games. To speed up the work you could consider modifying debug parameter in configuration file to 65535 giving you an extra powerful hero.
I would like to know that we didn't screw gameplay with these changes as the code is re-written from scratch.

It sounds like you are still working on this.
Are there any adjustments that need to be made before the change is in its "final" version?
I wouldn't want to start testing again.
Please tell me when it will be ready for testing.

Hi @LeHerosInconnu , I was addressing code related issues without touching the game's logic. Also right now all reviewers approved the changes so please proceed with testing once the current commit will be ready to be downloaded (in maximum 20 minutes).

@ihhub
Copy link
Owner Author

ihhub commented Jun 27, 2021

Hi @Branikolog , could you please also help with testing of these changes are they are very critical for the release?

@LeHerosInconnu
Copy link

Hello @ihhub,

Here are the differences I've spotted so far.
(One image from the original game with one image from fheroes2.)
There are many.
Passability differences 01 01.zip
Passability differences 01 02.zip
Passability differences 01 03.zip

@ihhub
Copy link
Owner Author

ihhub commented Jun 29, 2021

Hi @idshibanov and @oleg-derevenetz , based on feedback from @LeHerosInconnu and @Branikolog I modified the logic. I also added temporary field called _level to avoid being overridden while we set artifact/spell/monster quantity. Could you please take a look at these changes again?

Copy link
Collaborator

@oleg-derevenetz oleg-derevenetz 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 no obvious issues found, except one note regarding read & write an obsolete field.

}

StreamBase & Maps::operator>>( StreamBase & msg, TilesAddon & ta )
{
msg >> ta.level >> ta.uniq >> ta.object >> ta.index >> ta.tmp;
uint8_t temp = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose, version check will be added later to not write/read an obsolete field.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. I didn't want to increase this pull request even further.

@ihhub
Copy link
Owner Author

ihhub commented Jun 30, 2021

Hi @LeHerosInconnu and @Branikolog , could you please verify changes again? I addressed the majority of found problems. Not everything is identical to the original game but this implementation must be much closer to the original.

@sonarcloud
Copy link

sonarcloud bot commented Jun 30, 2021

@ihhub ihhub merged commit dcf2992 into master Jun 30, 2021
@ihhub ihhub deleted the object_passability branch June 30, 2021 14:12
@ihhub
Copy link
Owner Author

ihhub commented Jun 30, 2021

Hi @LeHerosInconnu and @Branikolog , for any other passability issues let's create a separate issue where we could define them. I found that the original game has some flaws in passability as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment