Although b4e8560698 made players instantly able to see each other on minimalist servers that did not change their cells from the default, it created problems with the default CoreScripts where players need to be logged in before receipt of a CellStates packet from them is taken into account, with the result being that a player was recorded as having loaded their initial cells on the server's C++ side but not on the Lua side.
It may simply be best to expect servers to set player cells.
Previously, turning off communication with the MasterServer from the server config and then attempting to use SetGameMode() or SetHostname() led to a server crash.
Since the beginnings of TES3MP, equipment packets have only been sent whenever an item has been replaced by an item with a different refId, with changes in an item's charge or count not sending a packet (but being included in the next packet sent as a result of a refId change). The reason for this was ostensibly the fact that every single equipment packet always included the details for all 19 equipment items (as per Koncord's original design decision), which would have led to massive packet spam if such a packet was sent every time you shot an arrow or lost a little bit of your armor's condition.
With minimalist equipment packets, it is now viable to send equipment packets whenever any item changes in some way, by having the equipment packet contain only that one item.
Previously, the index changes were not cleared at the start of their corresponding update functions, which in turn meant that an Attribute/Skill/StatsDynamic/Equipment packet received by a player from the server made that player send back the same packet, as the index changes from it were retained.
Additionally, exchangeFullInfo was not set to false, thus sometimes leading to constant full exchanges of information.
Previously, two players entering the same cell only sent and received their latest changes for dynamic stats, attributes, skills and equipment when they started sharing that cell.
Previously, whenever a single attribute value changed for a player, that player then sent a PlayerAttribute packet with all values for all 8 attributes.
This did not cause anywhere as much packet spam as PlayerSkill used to, but there was no good reason not to fix it as well.
(cherry picked from commit b0965f094a)
Previously, whenever a single skill value changed for a player, that player then sent a PlayerSkill packet with all values for all 27 skills, plus the player's progress towards the next level and the bonuses to each attribute on the next level up as the result of sklll increases thus far.
This commit makes PlayerSkill contain only the values of specific skills, moves the player's progress towards the next level to PlayerLevel packets, and moves the bonuses to each attribute on the next level up to PlayerAttribute packets.
Players now also send a PlayerSkill packet whenever their progress towards a new point in a skill changes. This was previously avoided so as to not have massive packet spam.
(cherry picked from commit ef79a98544)
Previously, an attempt by the server to simultaneously change a player's cell and skills (as you'd expect when a player file is loaded) led to:
1) The server sending the cell packet first and the skill packet afterwards
2) The player receiving the cell packet and sending their own skill packet as part of the client's forced skill update
3) The player receiving the skill packet from the server
4) The server receiving the skill packet from the player
The result was that, if the player then left the server without sending another skill packet, the server's memory retained the skills the player had sent instead of the skills it had sent to the player.
This is the first step in a solution to that situation and similar ones.
(cherry picked from commit cac4684986)
Note: In 0.6.x, this was only a problem if a player's cell was set by the server first and their skills were set next, i.e. this was not a problem in the default CoreScripts because the opposite order used there masked the problem. It was a more significant problem in 0.7 because all packets were queued for a player and sent in a specific hardcoded order.
Previously, after finishing the TES3MP chargen once, mCreationStage was set to 4 in OpenMW, which in turn made it impossible to go through only specific chargen menus again as the result of the relevant TES3MP script function (tes3mp.SetCharGenStage(pid, startStage, endStage) in 0.6.3, player:setCharGenStages(startStage, endStage) in 0.7). In other words, trying to allow a player to just choose their class again made it so the player started at that menu and went through all the other subsequent menus as well, i.e. the player went through the class, birthsign and review menus.
Previously, charGenStage.end was doing double duty as both the variable indicating the number of CharGen stages and – when set to 0 – the variable indicating that CharGen was over. The latter role is now filled by a new boolean.
(cherry picked from commit 926106cf8c)
Previously, initial cell states were sent in LocalPlayer::processCharGen() and were ignored by the server because the player was not yet regarded as loaded. The result was that existing players logging in could not see each other until they went through at least one cell change.
(cherry picked from commit b4e8560698)
# Conflicts:
# apps/openmw/mwmp/LocalPlayer.cpp
cppcheck:
[apps/esmtool/record.cpp:697]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[apps/esmtool/record.cpp:1126]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[apps/esmtool/record.cpp:1138]: (performance) Prefer prefix ++/-- operators for non-primitive types.
[apps/niftest/niftest.cpp:36]: (performance) Function parameter 'filename' should be passed by reference.
[apps/niftest/niftest.cpp:41]: (performance) Function parameter 'filename' should be passed by reference.
[apps/opencs/model/prefs/boolsetting.cpp:25]: (warning) Possible leak in public function. The pointer 'mWidget' is not deallocated before it is allocated.
[apps/opencs/model/prefs/shortcuteventhandler.cpp:52]: (warning) Return value of std::remove() ignored. Elements remain in container.
[apps/openmw/mwstate/quicksavemanager.cpp:5]: (performance) Variable 'mSaveName' is assigned in constructor body. Consider performing initialization in initialization list.
PVS-Studio:
apps/opencs/model/filter/parser.cpp 582 warn V560 A part of conditional expression is always true: allowPredefined.
apps/opencs/view/world/referencecreator.cpp 67 warn V547 Expression '!errors.empty()' is always false.
apps/opencs/view/world/referencecreator.cpp 74 warn V547 Expression '!errors.empty()' is always false.
apps/opencs/view/doc/loader.cpp 170 warn V560 A part of conditional expression is always true: !completed.
apps/opencs/view/doc/loader.cpp 170 warn V560 A part of conditional expression is always true: !error.empty().
apps/opencs/model/tools/pathgridcheck.cpp 32 err V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 32, 34.
apps/opencs/model/world/refidadapterimp.cpp 1376 err V547 Expression 'subColIndex < 3' is always true.
apps/openmw/mwgui/widgets.hpp 318 warn V703 It is odd that the 'mEnableRepeat' field in derived class 'MWScrollBar' overwrites field in base class 'ScrollBar'. Check lines: widgets.hpp:318, MyGUI_ScrollBar.h:179.
apps/openmw/mwgui/widgets.hpp 319 warn V703 It is odd that the 'mRepeatTriggerTime' field in derived class 'MWScrollBar' overwrites field in base class 'ScrollBar'. Check lines: widgets.hpp:319, MyGUI_ScrollBar.h:180.
apps/openmw/mwgui/widgets.hpp 320 warn V703 It is odd that the 'mRepeatStepTime' field in derived class 'MWScrollBar' overwrites field in base class 'ScrollBar'. Check lines: widgets.hpp:320, MyGUI_ScrollBar.h:181
apps/openmw/mwmechanics/actors.cpp 1425 warn V547 Expression '!detected' is always true.
apps/openmw/mwmechanics/character.cpp 2155 err V547 Expression 'mode == 0' is always true.
apps/openmw/mwmechanics/character.cpp 1192 warn V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present.
apps/openmw/mwmechanics/character.cpp 521 warn V560 A part of conditional expression is always true: (idle == mIdleState).
apps/openmw/mwmechanics/pathfinding.cpp 317 err V547 Expression 'mPath.size() >= 2' is always true.
apps/openmw/mwscript/interpretercontext.cpp 409 warn V560 A part of conditional expression is always false: rank > 9.
apps/openmw/mwgui/windowbase.cpp 28 warn V560 A part of conditional expression is always true: !visible.
apps/openmw/mwgui/journalwindow.cpp 561 warn V547 Expression '!mAllQuests' is always false.
apps/openmw/mwgui/referenceinterface.cpp 18 warn V571 Recurring check. The '!mPtr.isEmpty()' condition was already verified in line 16.
apps/openmw/mwworld/scene.cpp 463 warn V547 Expression 'adjustPlayerPos' is always true.
apps/openmw/mwworld/worldimp.cpp 409 err V766 An item with the same key '"sCompanionShare"' has already been added.
apps/openmw/mwworld/cellstore.cpp 691 warn V519 The 'state.mWaterLevel' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 689, 691.
apps/openmw/mwworld/weather.cpp 1125 warn V519 The 'mResult.mParticleEffect' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1123, 1125.
apps/openmw/mwworld/weather.cpp 1137 warn V519 The 'mResult.mParticleEffect' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1135, 1137.
apps/wizard/unshield/unshieldworker.cpp 475 warn V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression.
apps/wizard/installationpage.cpp 163 warn V735 Possibly an incorrect HTML. The "</p" closing tag was encountered, while the "</span" tag was expected.
components/fontloader/fontloader.cpp 427 err V547 Expression 'i == 1' is always true.
components/nifosg/nifloader.cpp 282 warn V519 The 'created' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 278, 282.
components/esm/loadregn.cpp 119 err V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 112, 119.
components/esm/cellref.cpp 178 warn V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 175, 178.
components/esmterrain/storage.cpp 235 warn V560 A part of conditional expression is always true: colStart == 0.
components/esmterrain/storage.cpp 237 warn V560 A part of conditional expression is always true: rowStart == 0.
This avoids the following error when receiving repeated PlayerBaseInfo packets: "Error in frame: Invalid slot, make sure you are not calling RefData::setCount for a container object"
Additionally, only re-equip items as the result of a PlayerBaseInfo packet if resetStats is true (because of its side effect of auto-equipping items for the player).
Unfortunately, the handshake attempt limit implemented in 4ebfcc4a21 for 0.7 and then ported over to 0.6 in a3a341fee6 does not appear to work properly, which is why gathering more information on it is important.
Additionally, delete DedicatedPlayers who disconnect.
Previously, all disconnected DedicatedPlayers were still kept in memory, but never used again. There was code that suggested they were meant to be reused upon reconnecting, but that reuse had never actually been implemented, and would probably not be that useful anyway.
Additionally, print player guids using their string representations for consistency.
The creation and updating of DedicatedPlayer references remains very inelegant, but this commit is the first step towards fixing that.
To fix warnings:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/shared_ptr_base.h:588:8: warning: delete called on non-final 'NifOsg::ControllerFunction' that has virtual functions but non-virtual destructor
[-Wdelete-non-virtual-dtor]
delete __p;
^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/shared_ptr_base.h:595:4: note: in instantiation of function template specialization
'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<NifOsg::ControllerFunction *>' requested here
: __shared_count(__p)
^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/shared_ptr_base.h:1079:17: note: in instantiation of function template specialization
'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<NifOsg::ControllerFunction *>' requested here
: _M_ptr(__p), _M_refcount(__p, typename is_array<_Tp>::type())
^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/shared_ptr.h:129:25: note: in instantiation of function template specialization 'std::__shared_ptr<NifOsg::ControllerFunction,
__gnu_cxx::_S_atomic>::__shared_ptr<NifOsg::ControllerFunction, void>' requested here
shared_ptr(_Yp* __p) : __shared_ptr<_Tp>(__p) { }
^
/home/elsid/dev/openmw/components/nifosg/nifloader.cpp:242:39: note: in instantiation of function template specialization 'std::shared_ptr<NifOsg::ControllerFunction>::shared_ptr<NifOsg::ControllerFunction, void>' requested here
callback->setFunction(std::shared_ptr<NifOsg::ControllerFunction>(new NifOsg::ControllerFunction(key)));
^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/unique_ptr.h:78:2: warning: delete called on non-final 'MWGui::ResponseCallback' that has virtual functions but non-virtual destructor
[-Wdelete-non-virtual-dtor]
delete __ptr;
^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../include/c++/7.3.1/bits/unique_ptr.h:268:4: note: in instantiation of member function 'std::default_delete<MWGui::ResponseCallback>::operator()' requested here
get_deleter()(__ptr);
^
/home/elsid/dev/openmw/apps/openmw/mwgui/dialogue.cpp:58:23: note: in instantiation of member function 'std::unique_ptr<MWGui::ResponseCallback, std::default_delete<MWGui::ResponseCallback> >::~unique_ptr' requested here
PersuasionDialog::PersuasionDialog(ResponseCallback* callback)
^
Previously, a Container packet with a REQUEST action always made the client respond with the contents of all the containers in that cell.
The previous behavior now only happens for requests that have no WorldObjects attached, while requests that have WorldObjects attached get a reply with the contents of those specific containers.
A main priority in TES3MP development is to avoid making major changes to OpenMW code, so as to avoid merge conflicts in the future. Whenever avoiding potential conflicts seems especially difficult for the proper implementation of a particular multiplayer feature, that multiplayer feature is often put off until later or partially implemented with the intent of being revisited in the future.
Container sync is the perfect example. Previously, the OpenMW code for container actions was kept exactly as it was, with clients unilaterally accepting their own container changes as per singleplayer-specific code, with only the addition that clients sent container packets every time they made a change in a container, packets which were then forwarded unquestioningly by the server to other players. This meant that two players clicking on the same item in a container at the same time both managed to take it, thus duplicating the item.
Immediately after the packets were already forwarded, server scripts were able to check for incorrect changes, such as the removal of more items than should have existed in a container, but they had to send their own packets that attempted to fix what had already been accepted on the initial client and then forwarded to all clients, which was quite onerous in some scenarios, such as when a player on a slow connection immediately dropped items in the world after taking them from a container (which is why the default TES3MP serverside scripts made no attempt at sending corrective packets at all, preferring to expect the matter to be solved in a later C++ implementation).
This commit fixes item duping in containers by preventing container actions from initially running on clients and by ending the automatic forwarding of container packets by the server. Instead, clients now send container packets that act as requests for container actions, and serverside scripts have to forward these requests themselves. In other words, without a matching Container event in the server's Lua scripts, players are completely unable to affect containers for themselves or for others.
To forward a received Container packet, the following line must be used in a Container event in the Lua scripts:
tes3mp.SendContainer(true, true)
When an invalid action count is used in a container request, the serverside scripts can amend it using the following new function:
tes3mp.SetReceivedContainerItemActionCount(objectIndex, itemIndex, actionCount)
Thus, the serverside scripts are able to allow only container actions that are correct based on their own recorded contents for that container.
The OpenMW code allowing unilateral container actions in mwgui/container.cpp is now prevented from executing. When a player's container request is returned to them, code in mwmp/WorldEvent.cpp simulates those container actions instead.
David Cernat
Nelsson Huotari
Alf Henrik Sauge
tri4ng1e
Marc Zinnschlag
Andrei Kortunov
Bret Curtis
Capostrophic
scrawl
Capostrophic
Evgeny Kurnevsky
Andreas Sturmlechner
Koncord
elsid
Ilya Zhuravlev
RoadTrain