Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Objects vanish after a while #230

Closed
adammal96 opened this issue Sep 14, 2017 · 31 comments
Closed

Objects vanish after a while #230

adammal96 opened this issue Sep 14, 2017 · 31 comments

Comments

@adammal96
Copy link

We are running into an issue where after a couple hours of our server running any newly placed or newly edited objects vanish directly after spawn. We have tried changing the settings back to default yet nothing has helped, we have roughly 250 static objects used as bases for when vehicles spawn on them, and this happens all over the map not just near other mapping where the object limit may be reached. We are kind of at a loss here as nothing really seems to be working.

@IstuntmanI
Copy link
Contributor

I had a similar bug when I used v2.9.0 for a short time (then gone back to v2.8.2). Pickups disappeared so I had to restart the server fast. You are using v2.9.0+, right ?

Probably related to the chunk/per-player tick changes too. Those were really major changes which weren't tested too much.

@adammal96
Copy link
Author

yes I am using the latest version of the plugin, I'll take a look at going back to 2.8.2 to see if that resolves anything

@IstuntmanI
Copy link
Contributor

Can you try the latest commit ? I added the files here.

@adammal96
Copy link
Author

Will do.

@raydex23
Copy link

raydex23 commented Sep 19, 2017

Something is really wrong. I've got similar problems since update to the latest stable version.
Newly placed objects are dissapearing after while, and their IDs are messed up.
GetItemStreamerID is often returning invalid object id even if i still see object.

@IstuntmanI
Copy link
Contributor

You should try the files I added for downloading too.

@raydex23
Copy link

3 days of testing your file, nothing changed. Same problems.

@adammal96
Copy link
Author

We've been testing it as well, our objects still vanish after ~ 24 hours

@adammal96
Copy link
Author

This also happens on version 2.8.2 as well, albeit it takes slightly longer. It only happens to objects that aren't spawned on server load, this also happens to objects that are edited. If I spawn a new object like a roadblock, they will vanish after a couple minutes, if I load a gate object on server start, it won't vanish unless I edit it, again with a couple minutes of staying. This also applies to Dynamic 3d text, if its moved it vanishes in a couple minutes

@IstuntmanI
Copy link
Contributor

That's weird. I'm using 2.8.2 on my server and it has no problem. Are you sure that you loaded 2.8.2 and not 2.9+ ? AFAIK nobody has similar problems with 2.8.2.

@raydex23
Copy link

2.8.2 is fine, must be your scripting bug.

@adammal96
Copy link
Author

Anything I should be looking at? I’m not sure what exactly could be wrong,

@samp-incognito
Copy link
Owner

2.8.2 is fine. This is the first I've heard of this happening in 2.9.1, though. Make sure you aren't mixing up the IDs in your own script (e.g., when you destroy an object, make sure you reset the variable pointing to that object in your script).

@adammal96
Copy link
Author

I've made sure I'm not mixing up ID's, it doesn't occur until after ~24+ hours of the server constantly running which puzzles me because if it was mixing up ID's it would happen all the time.

@IstuntmanI
Copy link
Contributor

IstuntmanI commented Oct 28, 2017

Can you give us more details of the moment when it happens ?:

  • return value of Streamer_CountVisibleItems( playerid, STREAMER_TYPE_OBJECT ) (the container may fail to get freed for some reason and the internal per-player visible items limit be reached and stop further streaming ?).
  • press F5 in-game and tell us the values of "RWObjects" and ObjectSlotsUsed (well, these should be normal, but just in case, I guess).
  • can you try a native CreatePlayerObject and see if it works (well, if the previous point's values are regular, this should work too) ?
  • try reconnecting and see if it works again, if it is still not working maybe even try connecting NPCs so you get a player ID which was never used before ? You will "get" some fresh/empty internal containers so streaming may start to work again, if a per-player container is at fault.

These are the main reasons the streaming loop may stop, so they may help.

@adammal96
Copy link
Author

I'll get back to you on the other items as those will require me to push an update however,
RWObjects is 877
ObjectSlotsUsed is 374
In pershing square by lots of mapping its at
RWObjects 952
ObjectSlotsUsed 647

@adammal96
Copy link
Author

adammal96 commented Nov 5, 2017

The streamer CountVisibleItems is around 100-200 when dynamic objects are disappearing, creating a static object results in it staying. It doesn't disappear like dynamics do. All the old dynamic objects don't despawn either, only if you create a new dynamic object or edit a current one will it disappear.

@IstuntmanI
Copy link
Contributor

IstuntmanI commented Nov 8, 2017

That's weird. Maybe Incognito can see something suspicious in that behavior, probably at the deleting part.

By the way, it looks like it isn't happening for all items at once, but it happens to random items types. It may happen only to objects, or only to pickups, or only to objects, most probably for more than one item type too. There have to be some common bits that are wrong.

@adammal96
Copy link
Author

adammal96 commented Nov 9, 2017 via email

@spacemud
Copy link

spacemud commented Nov 9, 2017

I'm experiencing a remarkably similar problem, also on 2.8.2. The objects don't spontaneously disappear for me, but rather get destroyed when other objects are spawned.

I've put debug prints at every point of my code where objects are created and destroyed and there is nothing to suggest that they are being destroyed from the script. The newly spawned objects often have the same ID as an existing object, suggesting that they are being 'overwritten' somehow. I'm very diligent about resetting data when I destroy an object, so doubt it is that. I will recheck though.

I will try some of the suggestions offered in this topic and report back.

@adammal96
Copy link
Author

adammal96 commented Nov 30, 2017

Just gonna bump this, still having this issue, I'm willing to try just about anything here. I may try what spacemud did and see if its overwriting. Just another thing, this happens with 3d Text Labels as well

@IstuntmanI
Copy link
Contributor

IstuntmanI commented Nov 30, 2017

The best idea would be to add tons of prints (sampgdk::logprintf, just like Pawn's printf) in Streamer Plugin in functions like processObjects, processMapIcons and so on I guess. I think that some temporary Streamer_ToggleItemProcessDebug( type, toggle ) would be nice (also a type parameter because AFAIK not all item types start being bugged at once, so we don't want debugging the working types at the moment, just the faulty one), so when you are no longer seeing any item type, you could enable it with some command and see tons of prints in the console and inspect it. Those prints should be placed at the start of processObjects, to see if it is actually checked, also after the first loop, before the creation (after discovering objects) and at the end to see if it actually ended (well, it should be printed always if the first one is printed) and of course some prints in those loops, also before some break; lines, also printing in those loops the sizes of player.internalObjects, discoveredObjects, existingObjects and that's all probably. If the first one isn't printed, then we should advance the debugging procedure to the Streamer::performPlayerUpdate function, but I guess that processObjects is actually called. Also, as I said in my first reply here:
I had a similar bug when I used v2.9.0 for a short time (then gone back to v2.8.2). Pickups disappeared so I had to restart the server fast. You are using v2.9.0+, right ?
the functions for pickups and actors should probably be debugged too, in case that the bug happens on those item types instead, even if they are globally streamed and not just per-player.

So yeah, the biggest problem is that no one knows why this happens and we can't easily add prints all over it, because we have to wait a lot of time. A function to enable those debugs would be nice so the console won't be spammed for every player when it is not needed.

That's what we need: someone to use it and debug everything he can.

@adammal96
Copy link
Author

I've started the process of adding print statements with object ID's, I will also do this for 3d text and pickups as well. I'll test this on 2.8.2 first and move to 2.9.0 if I find a result.

@IstuntmanI
Copy link
Contributor

v2.8.2 is fine, all you need to do is check them in the latest version and see what seems suspect when items stop appearing.

@IstuntmanI
Copy link
Contributor

IstuntmanI commented Dec 6, 2017

@samp-incognito I think I found the problem (EDIT: At a second thought this isn't looking like a big problem, it may be the same, but still should be fixed)... if this behavior isn't actually the expected behavior (they just keep the last value of the created variable when it has a shadowing name). I'll submit here all warnings I got:

==== Building streamer (debug) ====
streamer.cpp
In file included from src/core.h:22:0,
                 from src/streamer.cpp:19:
src/grid.h: In member function ‘void Grid::eraseCellIfEmpty(const SharedCell&)’:
src/grid.h:94:2: warning: declaration of ‘cell’ shadows a global declaration [-Wshadow]
  {
  ^
In file included from lib/sampgdk/sampgdk.h:165:0,
                 from src/common.h:76,
                 from src/cell.h:20,
                 from src/streamer.h:20,
                 from src/streamer.cpp:17:
lib/samp-plugin-sdk/amx/amx.h:151:21: note: shadowed declaration is here
   typedef int32_t   cell;
                     ^~~~
src/streamer.cpp: In member function ‘void Streamer::processMapIcons(Player&, const std::vector<boost::intrusive_ptr<Cell> >&)’:
src/streamer.cpp:876:47: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
      boost::unordered_map<int, int>::iterator i = player.internalMapIcons.find(e->second->mapIconID);
                                               ^
src/streamer.cpp:864:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalMapIcons.find(d->second->mapIconID);
                                            ^
src/streamer.cpp: In member function ‘void Streamer::processObjects(Player&, const std::vector<boost::intrusive_ptr<Cell> >&)’:
src/streamer.cpp:981:46: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
     boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->attach->object);
                                              ^
src/streamer.cpp:971:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->objectID);
                                            ^
src/streamer.cpp:996:47: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
      boost::unordered_map<int, int>::iterator i = player.internalObjects.find(e->second->objectID);
                                               ^
src/streamer.cpp:971:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->objectID);
                                            ^
src/streamer.cpp: In member function ‘void Streamer::processTextLabels(Player&, const std::vector<boost::intrusive_ptr<Cell> >&)’:
src/streamer.cpp:1291:47: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
      boost::unordered_map<int, int>::iterator i = player.internalTextLabels.find(e->second->textLabelID);
                                               ^
src/streamer.cpp:1279:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalTextLabels.find(d->second->textLabelID);
                                            ^
amx.cpp
In file included from src/utility/../core.h:22:0,
                 from src/utility/amx.cpp:19:
src/utility/../grid.h: In member function ‘void Grid::eraseCellIfEmpty(const SharedCell&)’:
src/utility/../grid.h:94:2: warning: declaration of ‘cell’ shadows a global declaration [-Wshadow]
  {
  ^
In file included from lib/sampgdk/sampgdk.h:165:0,
                 from src/utility/../common.h:76,
                 from src/utility/misc.h:20,
                 from src/utility/amx.h:20,
                 from src/utility/amx.cpp:17:
lib/samp-plugin-sdk/amx/amx.h:151:21: note: shadowed declaration is here
   typedef int32_t   cell;
                     ^~~~
src/utility/amx.cpp: In function ‘void Utility::executeFinalAreaCallbacks(int)’:
src/utility/amx.cpp:219:33: warning: declaration of ‘a’ shadows a previous local [-Wshadow]
   for (std::set<AMX*>::iterator a = core->getData()->interfaces.begin(); a != core->getData()->interfaces.end(); ++a)
                                 ^
src/utility/amx.cpp:205:56: note: shadowed declaration is here
  boost::unordered_map<int, Item::SharedArea>::iterator a = core->getData()->areas.find(areaid);
                                                        ^
geometry.cpp
In file included from src/utility/../core.h:22:0,
                 from src/utility/geometry.cpp:20:
src/utility/../grid.h: In member function ‘void Grid::eraseCellIfEmpty(const SharedCell&)’:
src/utility/../grid.h:94:2: warning: declaration of ‘cell’ shadows a global declaration [-Wshadow]
  {
  ^
In file included from lib/sampgdk/sampgdk.h:165:0,
                 from src/utility/../common.h:76,
                 from src/utility/misc.h:20,
                 from src/utility/geometry.h:20,
                 from src/utility/geometry.cpp:17:
lib/samp-plugin-sdk/amx/amx.h:151:21: note: shadowed declaration is here
   typedef int32_t   cell;
                     ^~~~
misc.cpp
In file included from src/utility/../core.h:22:0,
                 from src/utility/misc.cpp:19:
src/utility/../grid.h: In member function ‘void Grid::eraseCellIfEmpty(const SharedCell&)’:
src/utility/../grid.h:94:2: warning: declaration of ‘cell’ shadows a global declaration [-Wshadow]
  {
  ^
In file included from lib/sampgdk/sampgdk.h:165:0,
                 from src/utility/../common.h:76,
                 from src/utility/misc.h:20,
                 from src/utility/misc.cpp:17:
lib/samp-plugin-sdk/amx/amx.h:151:21: note: shadowed declaration is here
   typedef int32_t   cell;
                     ^~~~
src/natives/objects.cpp: In function ‘cell Natives::AttachDynamicObjectToObject(AMX*, cell*)’:
src/natives/objects.cpp:375:24: warning: declaration of ‘native’ shadows a previous local [-Wshadow]
      static AMX_NATIVE native = sampgdk::FindNative("AttachPlayerObjectToObject");
                        ^~~~~~
src/natives/objects.cpp:346:20: note: shadowed declaration is here
  static AMX_NATIVE native = sampgdk::FindNative("SetPlayerGravity");
                    ^~~~~~
src/natives/objects.cpp: In function ‘cell Natives::AttachDynamicObjectToPlayer(AMX*, cell*)’:
src/natives/objects.cpp:450:23: warning: declaration of ‘native’ shadows a previous local [-Wshadow]
     static AMX_NATIVE native = sampgdk::FindNative("AttachPlayerObjectToPlayer");
                       ^~~~~~
src/natives/objects.cpp:425:20: note: shadowed declaration is here
  static AMX_NATIVE native = sampgdk::FindNative("SetPlayerGravity");
                    ^~~~~~
src/natives/updates.cpp: In function ‘cell Natives::Streamer_ToggleItemUpdate(AMX*, cell*)’:
src/natives/updates.cpp:87:38: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
   if (static_cast<size_t>(params[2]) >= 0 && static_cast<size_t>(params[2]) < STREAMER_MAX_TYPES)
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
src/natives/updates.cpp: In function ‘cell Natives::Streamer_IsToggleItemUpdate(AMX*, cell*)’:
src/natives/updates.cpp:102:38: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
   if (static_cast<size_t>(params[2]) >= 0 && static_cast<size_t>(params[2]) < STREAMER_MAX_TYPES)
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

My suggestions:

  1. (unrelated) Change src/grid.h eraseCellIfEmpty's function argument name from cell to something like sharedCell, localCell, passedCell or whatever you like it to be called like, cell is already defined by the SDK to be int, so there's shadowing. Maybe also some other cell names, like the one from grid.cpp:
SharedCell cell(new Cell(cellID));
cells[cellID] = cell;

into (isn't it possible with no variable ?):

cells[cellID] = SharedCell(new Cell(cellID));
  1. More important: Fix all the other shadow warnings (mainly from src/streamer.cpp). I told you some time ago do so, but now I inspected them a bit and they look like a problem. One example of them:
src/streamer.cpp: In member function ‘void Streamer::processObjects(Player&, const std::vector<boost::intrusive_ptr<Cell> >&)’:
src/streamer.cpp:981:46: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
     boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->attach->object);
                                              ^
src/streamer.cpp:971:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->objectID);
                                            ^
src/streamer.cpp:996:47: warning: declaration of ‘i’ shadows a previous local [-Wshadow]
      boost::unordered_map<int, int>::iterator i = player.internalObjects.find(e->second->objectID);
                                               ^
src/streamer.cpp:971:44: note: shadowed declaration is here
   boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->objectID);
                                            ^

Here's the code:

boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->objectID);
if (i != player.internalObjects.end())
{
continue;
}
int internalBaseID = INVALID_STREAMER_ID;
if (d->second->attach)
{
if (d->second->attach->object != INVALID_STREAMER_ID)
{
boost::unordered_map<int, int>::iterator i = player.internalObjects.find(d->second->attach->object);
if (i == player.internalObjects.end())
{
continue;
}
internalBaseID = i->second;
}
}
if (player.internalObjects.size() == player.currentVisibleObjects)
{
std::multimap<std::pair<int, float>, Item::SharedObject, Item::PairCompare>::reverse_iterator e = existingObjects.rbegin();
if (e != existingObjects.rend())
{
if (e->first.first < d->first.first || (e->first.second > STREAMER_STATIC_DISTANCE_CUTOFF && d->first.second < e->first.second))
{
boost::unordered_map<int, int>::iterator i = player.internalObjects.find(e->second->objectID);
if (i != player.internalObjects.end())
{
sampgdk::DestroyPlayerObject(player.playerID, i->second);
if (e->second->streamCallbacks)
{
streamOutCallbacks.push_back(boost::make_tuple(STREAMER_TYPE_OBJECT, e->second->objectID));
}
player.internalObjects.erase(i);
}
if (e->second->cell)
{
player.visibleCell->objects.erase(e->second->objectID);
}
existingObjects.erase(--e.base());
}
}
}

You can see that the iterator i is created 3 times in the same scope (well, the first one has a bigger scope than the rest) with the same type: boost::unordered_map<int, int>::iterator, so everytime i is used it will get its latest value. I think that you will know how these should be fixed, I'm not sure if they should just keep their last created value like it does now (I didn't look too much, but for now I think that every i after a variable creation with the same name should use the same value, so it isn't exactly a problem for now).

After you will do this, if you want you can do this in the next commit, all you have to do is modify a bit the premake5.lua file and regenerate the files for vs2015 and gmake compiling:

  1. Change
    includedirs { "include", "lib", "lib/**" }
    to
    sysincludedirs { "include", "lib", "lib/**" }
    ( https://stackoverflow.com/questions/1867065/how-to-suppress-gcc-warnings-from-library-headers )
  2. Add
    buildoptions { "-Wextra", "-pedantic", "-Wconversion", "-Wcast-align", "-Wcast-qual", "-Wctor-dtor-privacy", "-Wdisabled-optimization", "-Wformat=2", "-Winit-self", "-Wlogical-op", "-Wmissing-include-dirs", "-Wnoexcept", "-Woverloaded-virtual", "-Wredundant-decls", "-Wshadow", "-Wsign-promo", "-Wstrict-null-sentinel", "-Wstrict-overflow=5", "-Wundef", "-Wno-unused", "-Wno-variadic-macros", "-Wno-parentheses", "-fdiagnostics-show-option" }
    after
    targetextension ".so"
    and it should work just fine. Showing all the right warnings for now (nothing else shows up for me with all of these enabled, so it will be future-proof and warn us about any mistake in the code).

I could do a pull request with all of these things fixed, but I'm not sure if I should rename them how it looks like and you may not like how I name them, so it's better to be solved by you. At least fix the ones with the shadowing of i, a and cell, if you don't have time for more. If there will be any more warnings after that I'll just take some time and fix them. (hopefully I won't rename things into some names you won't like)

@samp-incognito
Copy link
Owner

samp-incognito commented Dec 7, 2017

I really doubt the shadowed variables are causing any problems in those specific cases. For clarity, though, yes, their names should be changed to something else.

if this behavior isn't actually the expected behavior (they just keep the last value of the created variable when it has a shadowing name

Shadowed variables can have different values as long as they're not in the same scope (https://en.wikipedia.org/wiki/Variable_shadowing#C++). This is legal, but it's not really considered good style, so I'll see about fixing all of the ones I can find later.

@adammal96
Copy link
Author

So the issue is currently happening. Still can't figure out why.

Line 85399: [09/12/2017 19:49:20] Generating Dyn Object 16477
Line 85405: [09/12/2017 19:49:36] Generating Dyn Object 16478

It appears to be assigning valid object ID's but they just disappear, they aren't getting overwritten or anything.

@IstuntmanI
Copy link
Contributor

@samp-incognito Well, I just thought that you are using the wrong variable because of the shadowing. I know that shadowing is legal. Well, I may do a pull request with everything I said.

@adammal96 If by "generating" you mean that CreatePlayerObject is actually called fine (so this means that they are discovered fine), then you should check if DestroyPlayerObject is called immediately after it. How often is this happening now ?

@adammal96
Copy link
Author

Happens every day, but only after like 15-20 hours. It prints the object ID when doing CreateDynamicObject, and prints when destroying but it doesn't destroy it, the object just vanishes

@IstuntmanI
Copy link
Contributor

I don't really get what you mean with the destroying part. If the DestroyPlayerObject part of code is called, it should be destroyed like it needs to. You mean that part isn't called at all ? Well, the object can't just vanish without any call to DestroyPlayerObject.

@adammal96
Copy link
Author

As far as I can see it doesn't get called. It just vanishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants