Closed topaz-next-bot closed 3 years ago
topaz-next-bot
commented
3 years ago 
_ Hurricane Wing in the air would be a TODO since I couldn't find the right skill for this. The standard wing is a ground animation
I believe the "super buff" move used by Nidhogg is supposed to be the aerial Hurricane Wings move.
topaz-next-bot
commented
3 years ago 
@Hozu Thank you for the pointer, I will check on that. ~~Edit: Does not seem to be the case. Super Buff is indeed a buff. Even though it appears next to skills that Wyrm uses I don't know when this would be used. Videos I have watched of the Wyrm fight do not show a similar animation (mob turns blue temporarily, like it was a 2h, possibly Mighty Strikes; the use of this skill also prevents use of other TP moves.) (Also see Teo's comment above about this)~~ Just for the records: Hozu was completely right, I just interpreted this based on less insight. The airborne Hurricane Wing got knowingly hijacked and overwritten at some point. This is now corrected.
topaz-next-bot
commented
3 years ago 
I've just gotten a Themis Orb on retail, so I'll be able to get caps of this soon. I just need to get someone to PL my /DNC so I can try solo it
topaz-next-bot
commented
3 years ago
So far so good. There's a few things that are not related to this PR that I observed and affects all wyrms in general:
Touchdown animation will not display even though the mob uses it
The magical default ranged attacks while flying have a huge range, which is nice, but easily provoke this behaviorint32 SetMobSkillAttack(lua_State*); // enable/disable using mobskills as regular attacks
Additionally as ibm and Teo mentioned, it is confusing that this uses a skill list ID instead of a skillglobal/mobskills/ return 1 if the AnimationSub is not 0
If any wyrm at any point has its AnimationSub changed to something else (and there's several bits for grounded mode), or a different default it will not use any of these skills
This is desirable when flying but Wyrm had a default of 6 (ground) for example and the skill Touchdown sets it to 2 after executingI created a well commented but very dirty workaround for the TP issue which after a lot of testing feels right during the fight and is better than no TP move at all during the flight phase.
TO DO for this PR:
topaz-next-bot
commented
3 years ago 
Set natural magic resistances (Fire & Ice) and weaknesses (Water & Thunder); I'm simply unsure where to do this
this is currently handled by the mob family table row
topaz-next-bot
commented
3 years ago 
SetMobSkillAttack functions how it should in that some mobs use multiple mobskills for their regular attack (ex: Shinryu, August). It should definitely be renamed to be more explanatory.
It should be functioning without needing 3000 TP. If it's not, something is broken.
topaz-next-bot
commented
3 years ago
For some reason after Touchdown is used mobskills will be used when reaching 1000TP not 3000TP (as when engaging the fight)
Mobs use TP moves sooner depending on their HP. At 100% HP they won't use TP until they're at 3000 TP. The point at which they're forced to use TP is earlier as their HP% goes down (on retail, they use TP at 1000 when below 25%).
topaz-next-bot
commented
3 years ago
It should be functioning without needing 3000 TP. If it's not, something is broken.
@ibm2431 A humble print(mob:getTP()) says more than a thousand words:
https://streamable.com/ojrve0
This record was made on a vanilla release build (last upstream merge last week) and with the TP hack removed. You can clearly see how once AnimationSub(1) plays and SetMobSkillAttack() kicks in I'm spamming ranged attack to no avail. Each time a regular attack is used the TP drop to 0.
Hope this helps and maybe someone could check on this.
On this note I would also like to point out that onMobFight() fires way too often if one can believe the description in luautils.h and luautils.cpp where it says:
int32 OnMobFight(CBaseEntity PMob, CBaseEntity PTarget); // Сalled every 3 sec when a player fight monster
(see comment below) But I still have to make this note that my server calls this 2 to 3 times every second, not every 2400ms (2.4s)
topaz-next-bot
commented
3 years ago
A humble print(mob:getTP()) says more than a thousand words:
And even if the mob has zero tp mobskill as melee will still go off at its specified melee delay interval. That it is erroneously removing tp when it "melee's" is a bug to be fixed, if that is happening (I believe it is).
On this note I would also like to point out that onMobFight() fires way too often if one can believe the description in
it fires every 2400ms 400ms exactly, and this is intentional. The comment was written by someone else that I guess never saw the code that times it. (combat tick is running off the universal server tick rate in CTaskMgr)
Edit: intentional and retail accurate too I forgot Kj changed it. The global timer still happens at 2400, but mob ai ticks at 400ms.
topaz-next-bot
commented
3 years ago
Latest commits add and correct the following:
onSpawn() because mod_modifier.h says so:
// 1 - player draw in, 2 - alliance draw in -- only add as a spawn mod!MOBMOD_SKILL_LIST is now used to set the corresponding new skill set while flying and then restores it after landingTO DO:
topaz-next-bot
commented
3 years ago
Latest commits add and correct the following:
For reference see here, here and here
TO DO:
topaz-next-bot
commented
3 years ago
@TeoTwawki Would appreciate a last review for any errors. As far as mechanics go, this is pretty much done now.
topaz-next-bot
commented
3 years ago
regression
Edited skill list so only Dragon Breath is in it: https://youtu.be/YogcdPTTKS4
Did you rebuild?
topaz-next-bot
commented
3 years ago
Thank you very much for getting back and even providing proof of work @ibm2431 , and even more for pointing me in the right direction. It's the first time I had a newly added global function affect a PR of mine. I should have read way more thoroughly through your PR when it conflicted with this. I apologize.
My build simply didn't find that function of yours. So today I pulled everything in, rebuilt and all is good. I also adapted the Wyrm skills from this PR to use your function instead of the old isBehind() and tested them positively.
Also found a typo I made in the database that was fixed.
If you don't see anything else I'd say this is ready now.
topaz-next-bot
commented
3 years ago
@reviewers Could this be tagged as reviewed and/or approved for merge? This works very well by now, no more changes have been requested. It's just sitting here.
topaz-next-bot
commented
3 years ago
Please remember that it's the weekend and staff/reviewers are volunteers. This will get re-reviewed and/or tested when someone has time.
topaz-next-bot
commented
3 years ago 
dunno if this needs to be tested maybe it could be merged into canary? have a few lv 99 players that can try the fight. Wish we had lots of players to try it on an alliance at 75.
topaz-next-bot
commented
3 years ago
GitHub unfortunately doesn't let non-organization members tag the project-topaz/reviewer role. This is a different group of people from reviewer or developer on GitHub. (Those tags do work in our Discord, though!)
Zach and I are the only official developers with write access, and PR reviews and merges are only a fraction of what we do. The reason that some PR reviews and merges take time is because we spend that time on things which are higher priority. There's a lot of history behind this - I've struggled with Zircon many, many times on this very subject - but slower reviews is the current solution to the problems the project faces. It's not from lack of effort - Zach and I spend more time on this project than anyone should.
Don't worry - I know you haven't claimed it was, MarianArlt. But I've been hearing of low-key complaining recently (not necessarily from you in particular), and this happens to be the most relevant public place for me to say the above since you mentioned review times.
kain, I know that you know how our process works. If you want Canaria to have new content sooner, you're going to have to start cherry-picking, merging your own work, and rebranding yourself as no longer being vanilla canary branch. We're not going to be pressured to merge PRs sooner for the benefit of players on a private server.
Onto the contents of the PR: I am killing fake "super buff" on any wyrms. I know the history of how it came to be. I do not care about said history, who did it, or the reasoning why. We're not assigning stuff to arbitrary non-captured skill IDs with the hope it won't cause a problem later. It was a boneheaded decision then because of this exact problem it's posing now.
The rule I am imposing going forward is that if it's not seen in a capture from the mob a skill is being assigned to, the skill does not get used, period. It doesn't matter if the skill "appears" free. If it hasn't been captured, it's not used. I am so tired of this codebase "faking it" leading to problems down the line (yes, this is the paralysis bitterness talking). It either gets done properly, with evidence we can cite, or not at all.
In the context of Nidhogg, we can:
If this results in the loss of a 2 hour animation until we better understand the mechanic behind the animation, then so be it. This fake "super buff" skill has already wasted more of our collective time and energy than it should have - time and energy which would be better spent elsewhere.
topaz-next-bot
commented
3 years ago
I appreciate your comment and everything in it. I hope you saw my Discord comment, I totally understand how much work you guys must have. I did think there were more people reviewing though actually. Thanks for clarification. Since I innocently just moved "super buff" here in this PR, should I rather revert the line where I put it back to original and only keep flying hurricane wing where it belongs? I don't feel this PR should be made responsible for this and resolve Nidhogg on the fly, so to say. Would appreciate further instructions, even if it was "handle Nidhogg please". Could also remove it here as mentioned and open a new issue to document the change and have it there for later.
topaz-next-bot
commented
3 years ago
Project Meta:
Official project staff are Zircon, Wiggo, Zach, and myself. Since GitHub doesn't allow us to easily list organization/group members, we keep a list of official staff and non-official "contributor groups" in our Discord's #resources channel. Wiggo is currently absent (life circumstances), but he stays out of development anyway (he prefers research). Zircon - as our official project lead figurehead - doesn't allow themselves to make development decisions so there isn't an implicit power imbalance should potential development arguments come up. That leaves just me and Zach at the moment as official write-access developers, absent more official staff.
Some contributors are part of our "organization" so they can be placed in non-official-staff contributor groups (project-topaz/developer, project-topaz/researcher) and can get pinged on things that might interest them ("hey, developers, we're doing this major change", "is there a researcher that might want to look into this?). This has the unfortunate side effect of badging them as a "Member". So while I do love Teo (and others), they don't have merge/write access. And as before, the mentions don't work on GitHub for anyone who isn't member of the parent organization (no doubt to kain's dismay 😉 )
We've been bouncing around ideas of giving such contributors more ways to assist the project (without giving them full repository write access), but GitHub makes this very difficult (as usual). So our ideas on that front have been stalled pending setup of more automated actions/bots.
Actual PR:
If Skill ID 956 is the flying version of hurricane wing (as seen from a capture), then it should stay as what you set it as.
If Skill ID 950 is the flying flame blast (again, from a capture) then that should also be kept to what you set it to.
Unless there's been a capture of Skill ID 949, it shouldn't be used for anything at all. If this means Fafnir no longer has a "spare" skill ID for the fake super buff move, that's fine, because we won't be using one. So you can revert Skill ID 949 back to what it used to be. I presume it was previously named Flame Blast because that's its name in the DAT.
We can fix Fafnir to no longer use the completely fake "super buff" skill (currently in skill ID 956) in a separate PR. We may want to time approvals / merges so they both go in at the same time. I'd like a volunteer for handling Fafnir's buffing properly, but if there isn't one soon-ish I can try to find some time (but I've also been trying to focus on some zone performance work recently).
topaz-next-bot
commented
3 years ago
@ibm2431 yeah I unserstand staff has higher priorities, I was just offering to somehow help if this was due to needing some testing. I think Instead I'll build it locally and provide feedback from there =)
My apologies if my comment felt as I was pressuring this PR.
topaz-next-bot
commented
3 years ago
the mentions don't work on GitHub for anyone who isn't member of the parent organization
@ibm2431 additional reason I'm unlikely to notice it:
Subfolder in my inbox.
That 112 is not even a full weeks worth of unread github notifications. One mention tends to get very buried so even if I did get those, I'd be sure to miss it since I subbed to the projects general notices.
topaz-next-bot
commented
3 years ago
I'll retarget to a version with post-super-merge release. The current target was branched from before I merged it.
Hey Teo, I'm remembering to retarget the open PR before deleting the target branch this time. 😉
topaz-next-bot
commented
3 years ago
I'll go through and review this later today 👍
topaz-next-bot
commented
3 years ago
I affirm:
Was missing spells: Absolute Terror, Horrid Roar and Flame Blast Had a wrong default subanimation in the database blocking all skills Flame Blast has a messed up animation ID, all three versions. I adapted the animation ID of Inferno Blast, they're exactly the same Does draw-in with the known perk of being able to pop out of bounds when drawing in while flying, like Tiamat Opens the battle with a mobskill Goes airborne at 66%HP and then evades all melee attacks Uses a different fire magic based ranged attack while in air as well as Flame Blast and Hurricane Wing Hurricane Wing in the air would be a TODO since I couldn't find the right skill for this. The standard wing is a ground animation Flame Blast can be used only while in air and with full TP Uses Touchdown at 33%HP and returns to all the standard wyrm attacks Also gains increase in Eva, Def and M.Def after landing until death Code is commented where it makes sense
The drops are already working so this would make Early Bird Catches the Wyrm complete.