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

Refactor path.cpp #79

Closed

Conversation

silverhammermba
Copy link
Contributor

@silverhammermba silverhammermba commented Jun 26, 2018

Refactored path-finding algorithms, named variables, added comments (based on my understanding).

Everything is logically equivalent to how it was before, with one exception: there was a bug in path_solid_pieces that let the player cut corners when moving down or right. I'm not sure how this sneaked into the reversed code, but the bug isn't present in my retail copy of Diablo so I fixed it.

Smoke tested by playing for a while without crashes. Also made sure that the maximum path length (24) matches retail Diablo.

@mewmew
Copy link
Contributor

mewmew commented Jun 26, 2018

Great work @silverhammermba!

@ghost
Copy link

ghost commented Jun 26, 2018

There were a few decompiler issues on path_solid_pieces (hence the "check" comments). Most of the time it's with multi-dimensional arrays that use subtraction in the index. Could you provide a map seed and location so I can test this bug? (Press 'r' in game to fetch it)

This will be merged into a temporary branch and reviewed later. But I can't promise everything will make it's way in. As dumb/pointless as it sounds, even though I'm the minority here, I'm trying very hard to keep things identical to the original. So simplifying code and removing/adding stuff breaks the whole point of a clean-reversal. A lot of people come to this project specifically because it documents the exact code that made history in 1996.

All that said, I'll be updating the readme soon to include contribution guidelines and some short-mid term goals. Changes should be kept as small as possible (ASAP 😝) for the time being. If you haven't noticed, most the of the edits I've made fix critical bugs/issues by only changing 1-5 lines. We're not it any hurry, there's plenty of time to clean things up later on once we've settled the priorities.

Reverse engineering is like a puzzle. It's like pulling apart every molecule, every atom and quark that makes our existence possible. Understanding it and putting it back together exactly the same way; just so we can gasp in astonishment when we have played god...

@silverhammermba
Copy link
Contributor Author

I figured it might be too dramatic a change.

I can make a more conservative PR that just fixes obvious decompiler issues, adds comments, and renames variables. Does that sound good?

I can provide a map seed, but it's pretty easy to test the bug. Find any 1x1 solid tile (e.g. pillar/brazier) and try walking around it in both directions. In Diablo you have to take 8 steps, in devilution you take 7.

@ghost
Copy link

ghost commented Jun 26, 2018

Your comments in the code were indeed very helpful, much appreciated. The PSX symbol unfortunately does not contain function names/variables for pathing, since it was optimized for the controller.

The change to make PATHNODE path_2_nodes[300] a single pointer seems to be correct. However, the decompiler states that should be taking up much more space. I'll rerun path_solid_pieces through the decompiler and see if it can't be fixed in the minimalist way. It appears dPiece[dx-4][dy] should likely have used -1 instead.

Edit: Also, I will be uploading the symbol files soon, so everyone can take a peek. Devilution's core aspect is based strictly on them, as they are essentially a "guide" to reversing the game. It contains even as much as total lines/line numbers and location of braces for each function!

@ghost
Copy link

ghost commented Jun 26, 2018

Ok, so just as expected it was my fault when putting dx-4. I was thinking int vs. char and used 4 as the size, but that's fixed now. Also re-decompiled and fixed some issues: galaxyhaxz/devilution@dcd2d3d

There's a gap of about 15,600 bytes in between path.cpp and pfile.cpp that's unused. Weird.

Feel free to add some of your comments and names to the new code, it should resolve any bugs.

@mewmew
Copy link
Contributor

mewmew commented Jun 27, 2018

Reverse engineering is like a puzzle. It's like pulling apart every molecule, every atom and quark that makes our existence possible. Understanding it and putting it back together exactly the same way; just so we can gasp in astonishment when we have played god...

Should add a quotes.md for this repo <3

@mewmew
Copy link
Contributor

mewmew commented Jun 27, 2018

Edit: Also, I will be uploading the symbol files soon, so everyone can take a peek. Devilution's core aspect is based strictly on them, as they are essentially a "guide" to reversing the game. It contains even as much as total lines/line numbers and location of braces for each function!

On a related note, @Predelnik developed a documentation generation tool for the sanctuary/notes repo, which is capable of producing IDC scripts that can import these notes into IDA. Travis has now been configured to automatically update the notes.idc script for ever push to the notes repo; so the latest version is always available at:

http://sanctuary.github.io/notes/notes.idc

To import the variable names and function signatures of the notes repo into a fresh IDB database for Diablo.exe (version 1.09b), simply download the IDC script, and follow these steps from IDA:

  • File -> Script file -> notes.idc

Note: This IDC script should only be applied to empty/new IDB databases, as it removes all names of functions and variables before adding new ones.

@ghost
Copy link

ghost commented Jun 27, 2018

To import the variable names and function signatures of the notes repo into a fresh IDB database for Diablo.exe (version 1.09b), simply download the IDC script, and follow these steps from IDA:

Wow that's great! I'd imagine once the cross-reference spreadsheet is finished, it shouldn't be difficult to parse so we can also import multiple versions!

@mewmew mewmew mentioned this pull request Jun 27, 2018
@ghost
Copy link

ghost commented Jun 28, 2018

We'll have to revisit this in the future. Symbols have been uploaded for reference.

@ghost ghost closed this Jun 28, 2018
This pull request was closed.
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