Login
User Name:

Password:



Register
Forgot your password?
Vote for Us!
Bug in disarm( )
Nov 12, 2017, 6:54 pm
By GatewaySysop
Bug in will_fall( )
Oct 23, 2017, 1:35 am
By GatewaySysop
Bug in do_zap( ), do_brandish( )
Oct 18, 2017, 1:52 pm
By GatewaySysop
Bug in get_exp_worth( )
Oct 10, 2017, 1:26 am
By GatewaySysop
Bug in do_drag( )
Oct 8, 2017, 12:40 am
By GatewaySysop
LOP Heroes Edition
Author: Vladaar
Submitted by: Vladaar
Heroes sound extras
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.3
Author: Vladaar
Submitted by: Vladaar
Memwatch
Author: Johan Lindh
Submitted by: Vladaar
Beastmaster 6D sound files
Author: Vladaar
Submitted by: Vladaar
Users Online
CommonCrawl, Yandex, DotBot

Members: 0
Guests: 4
Stats
Files
Topics
Posts
Members
Newest Member
476
3,704
19,231
608
LAntorcha
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » Codebases » SWFOTE FUSS » destroy_ship sends ships to v...
Forum Rules | Mark all | Recent Posts

destroy_ship sends ships to vnum 46
< Newer Topic :: Older Topic >

Pages:<< prev 1 next >>
Post is unread #1 Dec 28, 2008, 11:43 am
Go to the top of the page
Go to the bottom of the page

ayuri
Magician
GroupMembers
Posts239
JoinedJun 13, 2008

Found by Caius/Ayuri

In space.c function destroy_ship find:
      ship_to_room( ship, 46 );
      ship->location = 46;
      ship->shipyard = 46;


Looks like this could cause a problem with ships being called '(null)' if you look in room 46.

Ayuri
       
Post is unread #2 Dec 28, 2008, 4:35 pm
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

Isn't this a standard practice of sorts? A room is designated as a 'trash can' and various junk things are sent there. Of course, the number '46' shouldn't be hard-coded like that -- it should be a constant -- but I'm not sure it's a bug per se.
       
Post is unread #3 Dec 28, 2008, 5:00 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

It's a bug if room 46 vanishes for some reason :P
       
Post is unread #4 Dec 29, 2008, 12:31 am   Last edited Dec 29, 2008, 12:53 am by ayuri
Go to the top of the page
Go to the bottom of the page

ayuri
Magician
GroupMembers
Posts239
JoinedJun 13, 2008

But what if what gets sent to vnum 46 isn't removed from the game in a timely manner?
I don't like things that show (null) when you look at them.
Test it yourselves. There was one case where I looked in room 46 and the game crashed. Granted - not many people would do such things but hey! Its fun when its fun!

Why not just get rid of it and be done with it?

As to standard practice, thats mostly for mppurge and like things right? Just dump the object/whatever somewhere and nuke it. But with ships? I'm not sure if thats a good idea. Isn't it dangerous just to have something like that floating about?

If I'm wrong, please enlighten me as to the error of my ways.

**EDIT**
I do have a core of the most recent swfotefuss 1.4 crashing at this.
#0  0x401d3283 in strlen () from /lib/tls/i686/cmov/libc.so.6
#1  0x401a2812 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
#2  0x401c3c04 in vsnprintf () from /lib/tls/i686/cmov/libc.so.6
#3  0x080f4eef in ch_printf (ch=0x867c440, fmt=0x821a794 "%-35s";)
    at color.c:1448
#4  0x08093601 in show_ships_to_char (ship=0x867ac38, ch=0x867c440)
    at act_info.c:786
#5  0x080a01fb in do_look (ch=0x867c440, argument=0xbfe75201 "";)
    at act_info.c:943
#6  0x08148b25 in interpret (ch=0x867c440, argument=0xbfe75201 "";)
    at interp.c:417
#7  0x08100fbf in game_loop () at comm.c:543
#8  0x081019db in main (argc=5, argv=0xbfe756d4) at comm.c:245
(gdb) frame 4
#4  0x08093601 in show_ships_to_char (ship=0x867ac38, ch=0x867c440)
    at act_info.c:786
786              ch_printf( ch, "%-35s", nship->name );
(gdb) p nship->name
$1 = 0x28004 <Address 0x28004 out of bounds>
(gdb) frame 5
#5  0x080a01fb in do_look (ch=0x867c440, argument=0xbfe75201 "";)
    at act_info.c:943
943           show_ships_to_char( ch->in_room->first_ship, ch );
(gdb) p ch->in_room->vnum
$2 = 46


I'll save all in a backup if anyone wants to debug that in depth.

Ayuri
       
Post is unread #5 Dec 29, 2008, 12:53 am
Go to the top of the page
Go to the bottom of the page

Caius
Magician
GroupMembers
Posts132
JoinedJan 29, 2006

Here's the deal. In SWR ships that are destroyed are sent to ROOM_LIMBO_SHIPYARD which is a macro for vnum 45. In FotE, however, ships are dynamically created and destroyed, so there's no point in sending them to any room at all. What happens in the original bug posted here is that destroy_ship() sends them to room 46 (not 45), then calls free_ship which deletes the ship. This leaves a dangling pointer in room 46's ship list, because it's never removed from that list.

So the correct solution is to never send it to room 46 in the first place. And if you really want to do this for whatever reason, then I recommend writing a macro like this...

#define ROOM_DEAD_SHIPS 46

... instead of using the magic number 46. This makes it easier in case you remove room 46 for whatever reason.

Another thing that might be done is to call extract_ship() in the free_ship() function. This would be a good idea in any case, since it eliminates the risk of having dangling pointers in a room in the first place.
       
Post is unread #6 Dec 29, 2008, 7:14 am
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

But what if what gets sent to vnum 46 isn't removed from the game in a timely manner?
I don't like things that show (null) when you look at them.

Arguably, the room is being used as an implementation detail of sorts, and nobody should be looking in there at all because the room isn't a normal 'room'. It's messy to have to do things like this, but if that's how things are set up ...

Caius said:

This leaves a dangling pointer in room 46's ship list, because it's never removed from that list. So the correct solution is to never send it to room 46 in the first place.

Why not just remove it from the room list?

Caius said:

Another thing that might be done is to call extract_ship() in the free_ship() function.

I don't like this personally since it is mixing engine logic into the "destructor". The destructor shouldn't be responsible for this kind of stuff. free_xyz should do nothing more than free the memory associated with the xyz object.
       
Post is unread #7 Dec 29, 2008, 8:30 am
Go to the top of the page
Go to the bottom of the page

Caius
Magician
GroupMembers
Posts132
JoinedJan 29, 2006

[quote=DavidHaley][quote]But what if what gets sent to vnum 46 isn't removed from the game in a timely manner?
I don't like things that show (null) when you look at them. [/quote]
Arguably, the room is being used as an implementation detail of sorts, and nobody should be looking in there at all because the room isn't a normal 'room'. It's messy to have to do things like this, but if that's how things are set up ...[/quote]
It serves no purpose to use this room. The ship is moved to room 46, then it's free'd right after. Could you please back up that "arguably" comment by referring to the actual FotE code? Also, saying that it's fine to have that dangling pointer because "nobody should be looking there" strikes me as very odd. How could it possibly be acceptable to have dangling pointers? Furthermore, it's possible to look in room 46, and hence someone will. Perhaps you meant something else?

[quote=DavidHaley][quote=Caius]Another thing that might be done is to call extract_ship() in the free_ship() function.[/quote]
I don't like this personally since it is mixing engine logic into the "destructor". The destructor shouldn't be responsible for this kind of stuff. free_xyz should do nothing more than free the memory associated with the xyz object.[/quote][/quote]
You could always wrap it all up into a function like cleanup_ship() (or whatever) and call both extract_ship and free_ship there if that makes you more comfortable.

[quote=DavidHaley][quote=Caius]This leaves a dangling pointer in room 46's ship list, because it's never removed from that list. So the correct solution is to never send it to room 46 in the first place. [/quote]
Why not just remove it from the room list?[/quote]
Good question. Which is why I keep saying that it shouldn't be moved there in the first place.
       
Post is unread #8 Dec 29, 2008, 8:47 am
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

I certainly didn't say it was fine to have dangling pointers; I said that it shouldn't be a problem to see "(null)" in garbage rooms. In other words, cosmetics don't really matter for implementation details. Obviously, the code should still be clean... I meant that if FotE works anything like some of the other FUSS bases, these "garbage rooms" exist to preserve integrity of iteration. There was a lengthy discussion about this on MB recently, so I won't really go into it. It would be worth investigating why these garbage rooms were created in the first place. It's obviously possible that somebody was just copying code without understanding it, and indeed there is no issue with iteration here.

Wrapping the "destructor" in a helper function is definitely better, but it still breaks the abstraction for it to be a ship-centric operation.
       
Post is unread #9 Dec 29, 2008, 9:15 am   Last edited Dec 29, 2008, 9:25 am by Caius
Go to the top of the page
Go to the bottom of the page

Caius
Magician
GroupMembers
Posts132
JoinedJan 29, 2006

DavidHaley said:

I certainly didn't say it was fine to have dangling pointers; I said that it shouldn't be a problem to see "(null)" in garbage rooms. In other words, cosmetics don't really matter for implementation details. Obviously, the code should still be clean... I meant that if FotE works anything like some of the other FUSS bases, these "garbage rooms" exist to preserve integrity of iteration. There was a lengthy discussion about this on MB recently, so I won't really go into it. It would be worth investigating why these garbage rooms were created in the first place. It's obviously possible that somebody was just copying code without understanding it, and indeed there is no issue with iteration here.

Yeah, you're right about the dangers of invalidating the room's ship list. In fact, the destroy_ship function calls itself if there are other ships in one of its hangars. Currently this is not problematic as such, because the ships are iterated on the global ship list, and not the hangar's local list.

And you're also right that it would be wise to investigate thoroughly. But as for the original topic there should be little doubt that the ship needs to be removed from room 46's ship list.

For the record, destroy_ship() is called in the following functions, neither of which removes it from room 46 afterwards:

move_ships()
update_space()
damage_ship_ch()
do_salvage() (moves ship explicitly to room 46)
do_clansalvage()
do_removeship()

As noted, do_salvage moves it to room 46, which is also done inside destroy_ship afterwards.

Furthermore, there are no other references to room 46 anywhere in the code. So obviously it's not used for anything.

Room 45 (the original limbo shipyard) is used once; in do_endsimulator() in case the simulator's ship->sim_vnum doesn't refer to an existing room. That's fine, of course. But it's using the magic number '45' rather than the ROOM_LIMBO_SHIPYARD macro. I suggest fixing this. Edit: ROOM_LIMBO_SHIPYARD is also used once, in fread_ship in case ship->shipyard is negative. It does not, however, check whether ship->shipyard contains the vnum of an existing room. Might want to fix this as well, and set it to ROOM_LIMBO_SHIPYARD if it's an invalid room.

My theory is that the garbage room is used simply because the coders at the time just followed the SWR way, without really thinking it over twice.
       
Post is unread #10 Dec 29, 2008, 3:00 pm   Last edited Dec 29, 2008, 3:03 pm by Keberus
Go to the top of the page
Go to the bottom of the page

Keberus
Conjurer
GroupFUSS Project Team
Posts341
JoinedJun 4, 2005

I believe to help clarify, this only became an issue after the free_ship function was put in. Before this, instead of freeing up ships, they were moved to vnum 46, and then unlinked from the ship list so they wouldnt be saved, then only after a reboot or copyover would the ship be deleted. Once freeship was put in, the need to just move a ship to a room to keep it 'out of the way' became unncessary. So I think that in this case removing the 3 lines that were suggested should be fine, because the ship has been free'd up anyways. I think it's just remnant code from how ships were removed before.
       
Post is unread #11 Jan 20, 2009, 10:19 am
Go to the top of the page
Go to the bottom of the page

ayuri
Magician
GroupMembers
Posts239
JoinedJun 13, 2008

One more thing, as Caius pointed out do_salvage also puts the ship into room 46. It would be best to remove that line of code as well.
In space.c do_salvage
        extract_ship( ship );
        ship_to_room( ship, 46 );
        destroy_ship( ship, ch, "salvaged" );
        send_to_char( "Ship removed.\r\n", ch );


Ayuri
       
Post is unread #12 Jan 20, 2009, 4:47 pm
Go to the top of the page
Go to the bottom of the page

Rojan QDel
Fledgling
GroupMembers
Posts25
JoinedDec 30, 2006

We took an alternative approach to this at LotJ. There is a separate linked list of destroyed ships, set with a certain flag so that they are loaded into the list of destroyed ships and not active ships. They are not usable, showshippable, etc. and they are stored in 45. However, the benefit to this is that we can have a restoreship command where destroyed ships can be restored much like a killed player can.
       
Post is unread #13 Jan 20, 2009, 6:06 pm
Go to the top of the page
Go to the bottom of the page

ayuri
Magician
GroupMembers
Posts239
JoinedJun 13, 2008

Rojan QDel said:

We took an alternative approach to this at LotJ. There is a separate linked list of destroyed ships, set with a certain flag so that they are loaded into the list of destroyed ships and not active ships. They are not usable, showshippable, etc. and they are stored in 45. However, the benefit to this is that we can have a restoreship command where destroyed ships can be restored much like a killed player can.


Very nice!

I'm guessing from what you posted that LotJ is not running a FotE codebase then - or a very modified one where you have player built ships/custom ships? (Sorry - I don't go searching for to many mud's these days. I've heard of LotJ but never stopped by)

As to us when it comes to restoring a ship for us - since nothing is special about the ship proto files - we just buy a new ship, give it the mods (if any) it had, and then set the owner to the player who we are giving it back to. No custom rooms/progs/whatever. Just what they had before. Very simplistic way of doing things.

Ayuri
       
Post is unread #14 Jan 21, 2009, 10:08 pm
Go to the top of the page
Go to the bottom of the page

Rojan QDel
Fledgling
GroupMembers
Posts25
JoinedDec 30, 2006

Yeah, LotJ is essentially its own custom codebase, far from SWR at this point (It is on an SWR base, not SWFotE).
       
Post is unread #15 Aug 28, 2009, 1:40 pm
Go to the top of the page
Go to the bottom of the page

Remcon
Geomancer
GroupAdministrators
Posts1,857
JoinedJul 26, 2005

This is an old thread, but since it was addressed might as well post the info here also.

in space.c
function destroy_ship
find
   if( !IS_SET( ship->flags, SHIP_RESPAWN ) && !IS_SET( ship->flags, SHIP_NODESTROY ) )
   {
      resetship( ship );
      size = ship->lastroom + 1;
      sroom = get_room_index( ship->firstroom );
      if( sroom != NULL )
      {
         tarea = sroom->area;
         for( vnum = ship->firstroom; vnum < size; vnum++ )
         {
            sroom = get_room_index( vnum );
            delete_room( sroom );
         }
         fold_area( tarea, tarea->filename, FALSE );
      }
      sprintf( shiplog, "Ship Deleted: %s", ship->name );
      log_string( shiplog );
      extract_ship( ship );
      ship_to_room( ship, 46 );
      ship->location = 46;
      ship->shipyard = 46;
      if( ship->starsystem )
         ship_from_starsystem( ship, ship->starsystem );

      sprintf( buf, "%s%s", SHIP_DIR, ship->filename );
      remove( buf );

      UNLINK( ship, first_ship, last_ship, next, prev );
      free_ship( ship );

      write_ship_list(  );
   }

before
      UNLINK( ship, first_ship, last_ship, next, prev );
      free_ship( ship );

add
      extract_ship( ship );

This fixes up the room 46 having all those NULL's.

Looks like you all had some interesting thoughts though.
       
Pages:<< prev 1 next >>