Pull requests cleanup

Everything about development and the OpenMW source code.
User avatar
Capostrophic
Posts: 652
Joined: 22 Feb 2016, 20:32

Re: Pull requests cleanup

Post by Capostrophic » 20 Feb 2019, 18:56

And now? ;)

User avatar
psi29a
Posts: 4630
Joined: 29 Sep 2011, 10:13
Location: Belgium
Gitlab profile: https://gitlab.com/psi29a/
Contact:

Re: Pull requests cleanup

Post by psi29a » 21 Feb 2019, 09:39

Right... queue up the PR/MRs that you guys think are "ready for merging", I'll take another look.

I've been hesitant to do anything because Zini is MIA, even with recastnavigation, I waited until his OK. However we're at a point now where waiting and being patient is no longer a virtue but a hindrance as PRs are starting to depend on other PRs.

User avatar
Capostrophic
Posts: 652
Joined: 22 Feb 2016, 20:32

Re: Pull requests cleanup

Post by Capostrophic » 21 Feb 2019, 17:18

PR 1929 — to follow TESCS compiler behavior, the rest of the line after an else operator is skipped so the compilation doesn't break due to arbitrary things in there (like invalid "else if" construction). Without a warning, though, exactly like in TESCS (adding a warning would have been complicated, but I'd like to adjust some script-related verifying functionality later anyway). Now, I know that you're not very familiar with the scripting system and Zini wanted to research into the approach first, but Zini is MIA and I still hope this could get a look on sooner than later because it's the single most important thing that "finishes up" my existing fixes for Sotha Sil Expanded incompatibilities (specifically actually unbreaking the falling pipe script and Interiors of Illusion script a lot of people have been complaining about).
PR 2068 — to follow Morrowind behavior, normal weapon resistance and weakness are only applied if both the ranged weapon and the projectile the weapon fires are considered normal.
PR 2069 — allows NPCs with a model set to actually use the skeleton of the model and not just the animations it introduces, replicating vanilla behavior and fixing some logged errors with Animated Morrowind, Arktwend and likely other things. Also allows Khajiit/Argonian NPCs to still look right even if their race has beast flag untoggled, even if the player looks weird.
PR 2082 — moves handling of disabled teleportation into the Mark/Recall/Intervention effect handling itself so that they are still added to the effect list briefly when they are cast without actually doing anything. It works in my testing and it doesn't seem like there would be any troubles with the approach.
PR 2107 — allows actors without AiWander to play idle voice lines. Allows creatures to play Attack voice lines (has no effect with vanilla assets and content files but has potential in modding). Currently has no changelog entries.
PR 2126 — further fixes to in-air running and sneaking stance usage. GetPCSneaking and GetPCRunning will return 1 and sneaking skill usage is possible when the player is falling or jumping when the player is in the respective stances.
PR 2146 — fixes SetJournalIndex and Journal behavior making it in line with vanilla, though if the player somehow gets the same status-changing index entry twice the quest status won't change. "Bailing out" behavior is messy at the moment anyway and will likely have to be refactored later.
PR 2163 is basically finished (has all the connected things I wanted to tweak in it) but I need to file an issue on the tracker.
PR 2168 is basically finished in the same sense but it likely needs more discussion.
PR 2172 — fixes some very minor issues found by PVS-Studio.
PR 2175 — generally reduces the number of includes in mechanics and world (and some dependent GUI).
PR 2177 — fixes the actor-visible-during-the-first-frame-of-being-loaded sometimes that has been bugging akortunov for a while. LGTM, I guess.
MR !57 — disables Preview for levelled list objects (which logically cannot be previewed) specifically.
MR !63 — fixes an inconsistency of override priority under MSVC leading to a unit test breaking and probably other things.
MR !64 — makes sure ESM reader context is always initialized before being used, fixing a unit test.
MR !65 — fixes a navigator unit test on Windows by using the real sizes of the necessary types instead of hardcoded values.

These changes are the ones I think are not extremely debatable.



User avatar
ElderTroll
Posts: 499
Joined: 25 Jan 2012, 07:01

Re: Pull requests cleanup

Post by ElderTroll » 08 Apr 2019, 02:57

Maybe time for new super code contributors to be given admin status or whatever the relevant title is for those who manage merges?
“A society grows great when old men plant trees whose shade they know they shall never sit in.”

User avatar
psi29a
Posts: 4630
Joined: 29 Sep 2011, 10:13
Location: Belgium
Gitlab profile: https://gitlab.com/psi29a/
Contact:

Re: Pull requests cleanup

Post by psi29a » 08 Apr 2019, 08:42

It isn't just 'merge' it is the review part that is important too. The person who created the PR/MR shouldn't be the one to hit merge, someone should at least review before hitting merge. There are exceptions of course, but that is our best practice. Kind of like the 'buddy system'. No one should go at it alone.

For the record, those who can merge are:
zini, kcat, davidc, myself, anyoldname3, ace, pvdk, corristo, capo, akortunov... might be more, just off the top of my head, that is 'the team' at the moment.

To give an indication of area expertise as it relates to OpenMW:
Zini: Architect / Lead Dev
Davidc: Lead Dev / Architect (due to tes3mp experience and lua expertise)
Anyoldname3: Lead Dev OSG/Graphics
Kcat: Lead Dev Music/Sound/OpenAL-Soft
Capo: General Dev (works on everything)
akortunov: General Dev (works on everything)
ace, corristo, myself/k1ll: platform champions/package maintainers
pvdk/myself: infra devs and maintainers

Umm... I sit oddly enough in a position of absolute power, yet with little time to really dig into OpenMW. I used to be able to spend days on end, but that time has flown. So I've kinda been filling in for Zini when trying to make decisions. I honestly do not feel comfortable doing this and when Lgro 'Chinese volunteered' me to take over infra... that is a fuck ton of responsibility so I enlisted pvdk's help. Don't get me wrong, OpenMW is extremely healthy, but I just felt it is necessary to be transparent as to who is doing what on the project so that other people that want to work on OpenMW know who to talk to and ask for help. :)

unelsson
Posts: 136
Joined: 17 Mar 2018, 14:57

Re: Pull requests cleanup

Post by unelsson » 08 Apr 2019, 12:19

I'm suggesting this for a merge, but another review doesn't hurt.
https://github.com/OpenMW/openmw/pull/1769

It can be improved though, and I have optimizations already developed in other branches, but I'd rather make a separate PR with them, as it requires touching the terrain storage system. Touching that affects more systems and should be checked properly before merge.

User avatar
akortunov
Posts: 618
Joined: 13 Mar 2017, 13:49
Location: Samara, Russian Federation

Re: Pull requests cleanup

Post by akortunov » 09 Apr 2019, 10:38


Post Reply