MrShecks / screep-studio

A cross platform client for the Screeps MMO sandbox game for programmers written in C++ and the Qt Cross-platform Framework as part of the Level1Techs "Devember2k18" coding event.
GNU General Public License v3.0
4 stars 0 forks source link

Incorrect adding of console lines #2

Closed Vort closed 4 years ago

Vort commented 4 years ago

ConsoleModel::addItems function have a problem. It removes items.length() + 1 items, but decrements count by items.length(), which results in this thing: ssexc I think calls to these functions should be fixed: https://doc.qt.io/qt-5/qabstractitemmodel.html#beginInsertRows https://doc.qt.io/qt-5/qabstractitemmodel.html#beginRemoveRows https://doc.qt.io/qt-5/qlist.html#erase-1 For example, like this:

diff --git a/models/console/ConsoleModel.cpp b/models/console/ConsoleModel.cpp
index 12af304..27f5932 100644
--- a/models/console/ConsoleModel.cpp
+++ b/models/console/ConsoleModel.cpp
@@ -192,18 +192,21 @@ void ConsoleModel::_onConsoleEventReceived(const SocketEventConsole& event) {
 //////////////////////////////////////////////////////////////////////////////////////////////////////////////////

 void ConsoleModel::addItems(const TItemList& items) {
+    if(items.length() == 0)
+        return;
+
     int count = _items.size();

     if(count > MAX_CONSOLE_ITEMS - items.length()) {
-        beginRemoveRows (QModelIndex(), 0, items.length ());
+        beginRemoveRows (QModelIndex(), 0, items.length () - 1);

-        _items.erase (_items.begin(), _items.begin() + items.length() + 1);
+        _items.erase (_items.begin(), _items.begin() + items.length());
         count -= items.length();

         endRemoveRows();
     }

-    beginInsertRows (QModelIndex(), count, count + items.size());
+    beginInsertRows (QModelIndex(), count, count + items.size() - 1);
     _items.append (items);
     endInsertRows();
 }
Vort commented 4 years ago

I was hoping that empty array will not be passed to this function. I was wrong. So additional check is needed. Edited my patch.

MrShecks commented 4 years ago

Thanks for reporting this issue and submitting your patch.

I have changed how the console model trims entries when MAX_CONSOLE_ITEMS has been reached so the bug should be fixed now.

Vort commented 4 years ago

@MrShecks please look at documentation for beginRemoveRows. If you need to remove 3 rows, starting from 0, then you need to remove rows 0, 1 and 2. last parameter in such case should be 2, not 3. That's why I was using - 1 in my patch. And that's why items.length() == 0 check is needed.

MrShecks commented 4 years ago

Sorry, you are correct, it is an off by one bug. I have corrected it locally and testing it now. Do you have a way to reproduce the case where ConsoleModel::addItems() is being called with an empty array?

There may be a bug in the NetworkModel if it is emitting the consoleEventReceived signal with invalid data.

Vort commented 4 years ago

Do you have a way to reproduce the case where ConsoleModel::addItems() is being called with an empty array?

My test colony just was destroyed. And remote code stopped to emit debug info. So any "empty" colony should emit "zero" arrays.

MrShecks commented 4 years ago

My test colony just was destroyed. And remote code stopped to emit debug info. So any "empty" colony should emit "zero" arrays.

Ok. I've only just rebuilt my test server and I am having trouble getting the official Screeps Steam Client to connect to it so I don't have anyway to get my own scripts onto the server, I only have some of the simple test bots running and because I don't own the rooms I don't get console output.

It will probably take me a while to get that working but in the meantime I will push the code to fix the off by one bug.

I don't think the server should be sending console events with empty info arrays but I will test this once I get my scripts onto my server, for the moment I have added a check for empty lists and added a Q_ASSERT to remind me to fix it if it happens.

If the assert is failing a lot for you please comment it out. I will leave the issue open.

Vort commented 4 years ago

It is not too hard to set up colony manually. Here are the corresponding parts from my client:

var jo = Request(false, "/register/submit",
    "{\"email\":\"your@email.com\",\"password\":\"your_password\",\"username\":\"YourNickname\"}");

jo = Request(false, "/game/place-spawn",
    $"{{\"room\":\"{room}\",\"x\":23,\"y\":28,\"name\":\"Spawn1\"}}");

var mainJs = File.ReadAllText("..\\..\\main.js");
JObject req = JObject.FromObject(new
    {
        branch = "default",
        modules = new
        {
            main = mainJs
        }
    });
jo = Request(false, "/user/code", req.ToString());
Vort commented 4 years ago

Hmm... I see no assertions with the new code. Which means that they may originate from enemy player. There were lot of enemy creeps when I was experiencing empty list problem.

MrShecks commented 4 years ago

Which means that they may originate from enemy player. There were lot of enemy creeps when I was experiencing empty list problem.

Interesting, that should be easy enough to test once I get my code up on to my server.

Thanks for the tips on the REST calls to push code to the server, if I can't get the steam client working I will add those calls to my API to push some test code.

Vort commented 4 years ago

One more useful API:

jo = Request(false, "/user/respawn", "{\"user\":{\"_id\":\"5ddfeec7ee0ae30288eadfa2\"}}");

It is enough to make a colony without logging actually.

Here is the message, which leads to assertion: sm

MrShecks commented 4 years ago

I managed to get my private server working and was able to re-create the issue with the empty console messages. Still not sure why the server is sending these, they seem at regular intervals like the game tick. Rather than filtering them inside the network code I've just ignored empty messages in ConsoleModel.

Vort commented 4 years ago

Console code looks good now. Thanks.