wheybags / freeablo

[ARCHIVED] Modern reimplementation of the Diablo 1 game engine
GNU General Public License v3.0
2.16k stars 195 forks source link

Feat/45/missiles #423

Closed grantramsay closed 4 years ago

grantramsay commented 5 years ago

Partial fix for #45/#46 (see #45 for discussion). Huge PR :O can split it up on request. I'll add comments to PR on main bits that concerned me. Uploaded a short video here.

To test click F5->F8 to cycle through active missiles, right click to fire/activate active missile.

wheybags commented 5 years ago

This sounds really great, but it's not super easy to grok at the moment. Could you write up a high-level overview of how the position and movement code works now, and also maybe a separate one on magic? Not just for me, I'd like to place them in the (underutilised) docs folder.

As for merging, I would like to get the issues with floating point trig functions sorted out before we do merge, as I don't want to break mp/save-load now, when I'm planning on getting 0.4 out soon.

Also, you mentioned you can split this up on-request, which would be great. I'll let you decide what the best logical boundaries are, just smaller chunks to review is easier in general :p Thanks very much! Super excited to see this shipping :D

grantramsay commented 5 years ago

Ok will add some docs.

As for merging, I would like to get the issues with floating point trig functions sorted out before we do merge, as I don't want to break mp/save-load now, when I'm planning on getting 0.4 out soon. How would floating point trig functions break multiplayer save/load? Fair enough if you don't want to risk breaking stuff for next release

Ok I can split it up, I might keep this one open for now for reference though. I think each initial (non refactoring / bug fix) commit are split into pretty decent logical boundaries:

Also I want to change missile ownership from level to actor, as:

AJenbo commented 4 years ago

spells like manashield need to follow the player across levels, so being bound to a level doesn't make sense

ManaShild was originally lost when moving between levels, but other than that I agree :)

wheybags commented 4 years ago

Thank you very much for this, and again, sorry for the prodigously long wait :v

wheybags commented 4 years ago

spells like manashield need to follow the player across levels, so being bound to a level doesn't make sense

ManaShild was originally lost when moving between levels, but other than that I agree :)

Do you have a list of spells that should and shouldn't follow players across levels? If so, could you make a github issue here to implement that mechanic appropriately?

AJenbo commented 4 years ago

spells like manashield need to follow the player across levels, so being bound to a level doesn't make sense

ManaShild was originally lost when moving between levels, but other than that I agree :)

Do you have a list of spells that should and shouldn't follow players across levels? If so, could you make a github issue here to implement that mechanic appropriately?

None of them follow you across levels, most of there state logic is tied to there missiles (all spells have at least one missile, but some are invisible).