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

Possible Delete Hero Implementation (redo) #186

Merged
merged 11 commits into from Jul 21, 2019

Conversation

danellos
Copy link
Contributor

This is a redo of an earlier pull request: a possible implementation for the Delete Hero feature, currently lacking as described in #61.

As before, the goal here was to avoid modifying Devilution's base code and focus this solution on DevilutionX. I want to avoid causing unnecessary merge conflicts until we're sure the game is on par with Retail.

On my previous pull request, a Travis build for macOS failed for i386. I am not sure why, but I was able to compile and run this on macOS Mojave 64-bit. If this fails again, I am happy to take another look to figure out why.

As before, two important notes:

  • In the base reference Diablo v 1.09 client, "Yes" is selected by default. In this solution, "No" is selected by default to avoid accidents. If you'd prefer to remain 100% consistent with the retail game, I can remove the code that does this.
  • The DeleteFileA() function was UNIMPLEMENTED(). I am not sure if you had a specific plan for how this would be implemented, but for now, I did it using the standard C approach. One difference from my previous pull request is I realized macOS can't resolve the file path, so I fixed that using the TranslateFileName() function helpfully provided already.

@danellos
Copy link
Contributor Author

danellos commented Jul 16, 2019

Fixed the macOS i386 build. Apparently the new cpp file needed to referenced in the Xcode project. Not sure why that wouldn't have failed the 64-bit build, but here we are :)

@AJenbo
Copy link
Member

AJenbo commented Jul 16, 2019

64bit builds use cmake like other platforms, 32bit mac uses a pre-written xcode project.

Copy link
Member

@AJenbo AJenbo left a comment

Choose a reason for hiding this comment

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

Nice work, will test it soon.

Regarding the defaults I'm a bit split on it, definitely for 1.1.0, probably for 1.0.0 i think.

@danellos
Copy link
Contributor Author

Noticed some minor inconsistencies with the Retail game, and fixed those. Let me know if you'd like me to remove the default "No" behavior.

@AJenbo AJenbo merged commit 903f136 into diasurgical:master Jul 21, 2019
@AJenbo
Copy link
Member

AJenbo commented Jul 21, 2019

Nice work, I made a couple of adjustments to get the font to align correctly and also enable keyboard navigation (a little bit of a hack though).

I also ended up defaulting to Yes for now, but I do think it's a really good improvement for 1.1.0, so good thinking.

@danellos
Copy link
Contributor Author

Awesome, happy to have made a contribution!

By the way, I re-tested building on both Windows (win32) and macOS Mojave (64-bit). The build processes were fine on both, so I am not sure what was happening there.

@AJenbo
Copy link
Member

AJenbo commented Jul 21, 2019

Not sure what you are referring to but I did break builds on master for a little bit today because of a bad merge with upstream. It's all fixed now.

@danellos
Copy link
Contributor Author

Ohhh gotcha, that might have been what I was looking at :) Thank you for all the work here! This project is very exciting.

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.

None yet

2 participants