Attnam / ivan

Iter Vehemens ad Necem - a continuation of the graphical roguelike by members of http://attnam.com
GNU General Public License v2.0
301 stars 43 forks source link

(See the other PRs first!) This is the merge of all my other PRs + some fixes and adjustments. #587

Open AquariusPower opened 4 years ago

AquariusPower commented 4 years ago

This was split on several PRs, but there is still code here that are not on them, so, let this be the last PR considered.


I think this is overall ready, I made some basic tests during implementations, and let it for hours on auto-play mode (but it does not test everything/flow). What is mainly missing is more tests on normal gameplay.

Many new things:

Bugfixes (some may only happen when using Wizard Auto Play mode):

And command line --help with some info and functionalities: --defgen Generate defines validator source file. (NEW) --defval Validate defines. (NEW) --version Show current game version.

Who have access to edit this PR, feel free to make any adjustments, even if minor.

Obs.: developer console is still broken tho but wont cause trouble if you dont use it. developer console is working again! I think cuz of this: https://github.com/Attnam/ivan/pull/589 :)

AquariusPower commented 4 years ago

There happened many crashes because of inconsistent character::TrapData linked list, during wizard auto play mainly at Spider Nest dungeon level.

How it gets inconsistent? Sometimes the trap is game::RemoveTrapID() just before the TrapData is used. This means that game::SearchTrap(TrapID) would return NULL for one of the traps on the TrapData. So everywhere TrapData was used directly (w/o checking if the trap still exists) could cause a SEGFAULT.

the solution The problem is many functions are const denying the use of the new ValidateTrapData() inside them. So I ended up calling this validation everywhere possible before such calls (what may feel messy...). It may impact performance? I think not. I am letting it auto playing (initially only at spider nest with 3 spawned lobh-se, 6 brown slimes and 6 green slimes, to have many simultaneous traps on same square) to be sure no more crashes will happen from TrapData inconsistencies. It will be in the next commit.

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging f487229146632c1b41317bfd60198c533c6baa03 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 507006c0dd7ea608cdbb368db6e7dbaa1c0659b1 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging ef990869e1bf5eac445a6d135f0dff1e6fdc581c into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 9b45b01835d98ef0195e308a347d4731d47564f1 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 68aeb84fddd3c421e4445c5977a70dbda79a90b3 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 510e8ce2d8f9ac077211d37e0f29498a1383523b into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 3218b0528fffbeebd3b9fcabf92f7c2d883db5c9 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 726d2d71e09dc91c8828a02850ce933da0f00181 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging ae8f8a8444119032be6c518c3dda147d036e3257 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

AquariusPower commented 4 years ago

I made a few changes at key handler (whandler) but could test only on linux.

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 0ee0e2fc526885927e85c17a5a35ec45d698cacb into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

ryfactor commented 4 years ago

I made a few changes at key handler (whandler) but could test only on linux.

You mean at 0ee0e2f? I can test it out on windows. Can you write a short list of brief tests? Looks like just typing and then backspacing into a text field?

AquariusPower commented 4 years ago

I made a few changes at key handler (whandler) but could test only on linux.

You mean at 0ee0e2f? I can test it out on windows. Can you write a short list of brief tests? Looks like just typing and then backspacing into a text field?

I got already shift keys that got broken, just fixed.

Yes, try some functionalities that requires typing a text like: the developer console, polymorphing, renaming a pet etc. Things that require a key combination like: shift+7 that is '&' and lets us summon monsters; the ctrl+'`' to open the console; ctrl+del to clear a list filter; F1 key on lists for help.

But... I think none will cause problems. I am asking mostly because any failures on key handler could mean chaos to the end user if we miss something :>. Basically play it a bit,even if on wizard mode,and see if anything looks broken.

What I have no idea how to test is all new possible keys that will be available at keybinding customization. My keyboard is a simple desktop multimedia one, nothing extra/reallySpecial on it...

ryfactor commented 4 years ago

Have you got a keypad? I tried changing the music volume to 100 via keypad and it simultaneously presses the End key so I get 127100 instead of 100

AquariusPower commented 4 years ago

Have you got a keypad? I tried changing the music volume to 100 via keypad and it simultaneously presses the End key so I get 127100 instead of 100

mmm... if the numlock is disabled it works fine, but if enabled (most cases here) despite it wont mess the volume value, what is shown there is messed up ex.: if you keep pressing keypad8 it fills up with 8s til the end

if I press keypad4 it feels like both LEFT and '4' are being generated for a single keypress! and that is really strange... I wonder if it already was like that before these changes? I will change the branch and recompile to see, thx!

AquariusPower commented 4 years ago

Have you got a keypad? I tried changing the music volume to 100 via keypad and it simultaneously presses the End key so I get 127100 instead of 100

yep, I tried the "old" autopickupregex branch (should try older one?) and the problem already existed, I think we can open a new issue to try to track why that is happening, but my guess is that it is specific to that control that accepts a directional and numbers, and probably the bug is on that control and not on the keyhandler code. That control seems a bit complex, it uses: scrollbaroption, BarHandler, NormalNumberChanger. If I am not wrong, all these interactions are providing the duplicated key recognizement (as direction and as number), we probably need some changes to let it only use direction or number, not both. EDIT: I think the keypad should always be numbers in this case and BarHandler should ignore them.

AquariusPower commented 4 years ago

~~the custom keybinding detects the INSERT key as 0x3FFF2049 (yeah a huge value) but all other keys are a low value, at most about 0x14F I created a pretty translator for these key values, showing ex. "End" for 0x14F, but I am in doubt if I should keep the same for the INSERT key (I found nothing else so weird like that on my keyboard tho). So my question is: your INSERT key also generates that value?~~

ugh... SDL key codes can be translated to FeLib ones, so I just granted SDL INSERT and DELETE detected scancode will always be translated to the same FeLib values and that is not a problem at all :)

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging b038c754115e3b69def1ffcf7df74c5ae9b22959 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 632b42884e3e77e6428fa680dee1e386ed343e77 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging d4329d8c73186322918e7bcbcccd901bc1d37f19 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 0cb782bf22f3eda6b50e28509c6334b573c1f802 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 716d6e5b89b943fa463032793c9d48faa7cb5b5e into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 7dcadf798e9483d74e52df702315882f30fab6f5 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging ec2b455a9c0aebbe5dae227392f9d5db2002f1e9 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

AquariusPower commented 4 years ago

@andrewtweber @fejoa @red-kangaroo

I added a new pre-processor definition: CURSEDDEVELOPER. The related code will only be compiled if CURSEDDEVELOPER is defined.

The point with this is that it lets the non wizard gameplay be tested by developers, but it wont be a normal gameplay. It will prevent player's death and prevent scoring (did I miss something else important?). It will show before player's name, on game screen: "[Cursed Developer!]".

Is this code ok? Should it be moved to a separate file (like I did with bugworkaround experimental code)?

Obs.: there is a trick also, the player's killer will be buffed everytime it succeeds.

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 21c43e13dff87b0afeed72c0b8a9b7bbd0a532e0 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging 0c3af2cb6a5d0c56ab25762820ca612bf00da92a into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging a00ebdf1b2848ecbad4e65338ebff276a71318fa into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging b8b6b1817cde2a3094c5f26e66ee2fbb90a2c383 into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

AquariusPower commented 4 years ago

This pull request fixes 9 alerts when merging b8b6b18 into 87f4a33 - view on LGTM.com

fixed alerts:

  • 6 for Too few arguments to formatting function
  • 3 for Declaration hides parameter

this lgtm-com BOT... https://github.com/marketplace/lgtm Why it keeps repeating useless messages? It helps ONLY when there is something to be fixed. When all lgtm tests are OK it should stop spamming these useless messages... Just a thought...

lgtm-com[bot] commented 4 years ago

This pull request fixes 9 alerts when merging af35eddc376c8a2dcdf0d5c157878d80b818416a into 87f4a33fc84e8b3b432c9db4211c3d213433cc98 - view on LGTM.com

fixed alerts:

ryfactor commented 4 years ago

Is this code ok? Should it be moved to a separate file (like I did with bugworkaround experimental code)?

@AquariusPower the extra code should be fine since it is a developer feature. Probably best to separate it into its own file if this is possible. Do you have any plans to split this PR into separate PRs based on features? That might make it faster to check and merge.

ryfactor commented 4 years ago

The lgtm bot only lets me ham-fistedly turn off comments for all PRs, which I have done. It will still run the analysis though.

AquariusPower commented 4 years ago

Is this code ok? Should it be moved to a separate file (like I did with bugworkaround experimental code)?

@AquariusPower the extra code should be fine since it is a developer feature. Probably best to separate it into its own file if this is possible. Do you have any plans to split this PR into separate PRs based on features? That might make it faster to check and merge.

sure, sounds good, I will try to split what I have not changed in a "long" time (days or weeks), like the crafting skill advancement I haven't touched it since after initial tests.

AquariusPower commented 4 years ago

The lgtm bot only lets me ham-fistedly turn off comments for all PRs, which I have done. It will still run the analysis though.

as soon I understood it, was easy and good to provide lgtm suggested fixes, it helps us with nice hints! ok I will try to remember to check LGTM Details link from time to time to see if all is ok, thx! :)

AquariusPower commented 4 years ago

Do you have any plans to split this PR into separate PRs based on features? That might make it faster to check and merge.

I was trying to cherry-pick commits using gitk, created a new branch "CraftSkill" (based on current Attnam-master), picked oldest about that subject, pushed it, went ok (https://github.com/AquariusPower/ivan/tree/CraftSkill).

When I tried to pick the 2nd (there is about 10 commits for it), it keeps giving me weird error messages, so I cant complete a full cherry pick for all 10 to complete that branch properly... I saw no reason for the errors... made no sense... probably some minor thing I am missing...

AquariusPower commented 4 years ago

yeah, I have no idea what to do, makes no sense, it wont allow me to cherry-pick anything else... error_009 I tried git reset made no difference I didnt try --allow-empty. As it is already empty why it is not being detected, like in, I didnt change anything else, it is already empty, why should I force allow empty? anyway, anyone knows how to easily cherry pick, even if in command line?

AquariusPower commented 4 years ago

ok used this: https://stackoverflow.com/a/3933416/1422630

git cherry-pick aa596fbd6a5d51da88c0ca5e8b7161406813c0b5^..82fe321c2a1255d45577784bf553683bffee8ac9

compiled, looks ok :> https://github.com/Attnam/ivan/pull/595

AquariusPower commented 4 years ago

Something new that I coded here seems to be creating a performance bottle neck. If left running in wizard auto play, in about 5 to 10 minutes, it gets somewhat slower, saving and reloading the game will fix the problem (but is annoying to do that). The problem may be related to traps validation (at spider nest mainly), not sure yet. I will try to run some profiller to track where could be the problem, but I never used one with C++, any tips are welcome. Anyway, may be many PRs is better (instead of this big one) so whatever is causing this problem may show up clearer. PS.: using a notebook to develop now, but I dont think that at the desktop pc it will be different, I will try there later anyway. EDIT: I am not really sure... it seems fast again... may be a hardware problem here.

AquariusPower commented 4 years ago

The main menu is a feature where each menu item can have it's own graphics, I just re-used the existing image but can be anything.

AquariusPower commented 4 years ago

Apparently the game per se is being successfully compiled by travisci, but it is happening some problem to compile IGOR and MIHAIL related to PCRE and SDL_MIXER.

I think changes need to be made at CMakeLists.txt (not sure which file I should edit, probably of those 2 sub-projects).

I never used these sub-projects, I am not sure what they are about. But I will try to fix their compilation (this probably means that SFX will work on them too).

red-kangaroo commented 4 years ago

@AquariusPower Does this branch contain all your changes, then? I'd like to try playing with all of them at once. :)

AquariusPower commented 4 years ago

@AquariusPower Does this branch contain all your changes, then? I'd like to try playing with all of them at once. :)

yup, this contains them all! I think this is the easiest way to test them too :>

AquariusPower commented 4 years ago

I am checking this conflict

red-kangaroo commented 3 years ago

Many nice changes here.

Screenshot from 2020-09-28 17-31-58 Screenshot from 2020-09-28 17-32-10

Really great work!

AquariusPower commented 3 years ago
* Stairs now look like stairs! I like it, though I will have to get used to that. ;) I've grown accustomed to the up/down arrows so long ago.

From the beginning I always felt strange a game with so many visual effects having arrows for stairs. But as I remember, the arrows are still there and it could be an option!

* As you scroll on the main menu, parts of the background image start to disappear:

Actually, there is one full background for each menu entry (it means each can be completely different), I actually thought a simple way to demonstrate it was to just remove parts of default background so I hadn't to draw anything new.

* Is it intentional that any keypress on main menu plays a random fighting sound? If it's intentional, can it be optional, please? :)

As I remember, each menu entry has a specific sound, and I just reused the existing sounds, yes there could have a config option to mute them easily. I actually thought better sounds could be used when moving on the main menu, or may be just a single non annoying sound just like a feedback that we pressed a key there.

* It would be nice if the god favours had F1 descriptions. I can try to write them, if you'd like.

cool!

* How does the red work in favour selection menu? It's the only menu that uses red in the whole game. Does it mean that the god has a timer/too low favour? I don't think it's necessary to warn the player, they would have already learned not to pray too often and it shouldn't be safe to call in a favour from a god.

as I remember, favours can only be asked if the player is aligned with that god's alignment OR if the player is on that god's sacred ground! So discovered favours from a god of other alignment cannot be called outside sacred grounds. They should show on the list but not necessarily in red, just inaccessible and greyed out. Red had a meaning, did you play in wizard mode? if so, that is probably it. I will check the code and add some comment or just disable the red.

* Favour names should probably be all capital, right now the capitalization is a bit inconsistent.

If I am not wrong, I typed them like titles (of movies for example) that only more important words are capitalized. Actually they should be written in other font to pass some kind of ancient/magic feeling. But it could be simplified, like may be set it all to lower or upper case.

* Favour cost should probably depend more on WISDOM than on MANA, as Wisdom is linked to your ability to negotiate with gods (it decreases prayer cooldown in IVAN, plus it was a primary stat for clerics in D&D where IVAN drew inspiration). Mana feels more like non-divine magic / magic item use, at least to me.

Agreed, the formulas should be reviewed!

* When reading the celestial monograph of Solicitus, you forget all gods and get your relation with them all set to 0, but right now you keep the knowledge of their favours. It should probably erase known favours, too.

I didn't know that, agreed!

* I don't think overloaded fighting should be on option. We should either change it to allow fighting while overloaded, or leave it as is, because that's a pretty important thing that can mean one character dies where another lives. Anyone else has an option on this? (We can do a vote on the forums?)

If I am not wrong, we just cannot kick, but there is really no reason to prevent using the arms! (1st vote cast :)) We shouldnt be able to dodge either, but I am not sure if that was coded, may be not..

Really great work!

thx!

red-kangaroo commented 3 years ago

Actually, there is one full background for each menu entry (it means each can be completely different), I actually thought a simple way to demonstrate it was to just remove parts of default background so I hadn't to draw anything new. As I remember, each menu entry has a specific sound, and I just reused the existing sounds, yes there could have a config option to mute them easily.

Ah, I see. That could be interesting with different background images, but an option to turn it off would be nice.

If I am not wrong, we just cannot kick, but there is really no reason to prevent using the arms! (1st vote cast :)) We shouldnt be able to dodge either, but I am not sure if that was coded, may be not..

I will ask on the forums, but I'm also in favour of changing it (2nd vote cast). :)

AquariusPower commented 3 years ago

Ah, I see. That could be interesting with different background images, but an option to turn it off would be nice.

actually, the easiest way to make it default right now to have just one image, is to copy the full one over the ones that have parts missing, the feature will remain active but will just not be visible. As soon someone comes with something cool, they just change the images and it will work!

Long ago, I always imagined the butterflies could actually fly, or at least blink or move their wings, but that would be independent of changing the focus on a menu item, it would require a refresh there also every 0.5s at least I guess.

red-kangaroo commented 3 years ago

BTW, majority of votes on forum is in favour of allowing attacks while overburdened.