Fishing script is broken

[Al Riddoch]

I get this error when initiating a fishing task:

2009-08-06T03:16:03 SCRIPT No bait in inventory
2009-08-06T03:16:03 ERROR Python error calling "tick_operation"
2009-08-06T03:16:03 SCRIPT_ERROR Traceback (most recent call last):
2009-08-06T03:16:03 SCRIPT_ERROR File "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Fishing.py", line 82, in tick_operation
2009-08-06T03:16:03 SCRIPT_ERROR for item in bait.contains:
2009-08-06T03:16:03 SCRIPT_ERROR NameError: global name 'bait' is not defined
2009-08-06T03:16:03 ERROR Script for "985" has reported an error processing a tick operation. This entity is probably now inactive.
2009-08-06T03:16:19 ERROR Connection(983) has found a character in its dictionery which is not connected.

[Al Riddoch (alriddoch)]

I also get this crash in a related script:

2009-08-06T03:24:30 ERROR Python error calling "tick_operation"
2009-08-06T03:24:30 SCRIPT_ERROR Traceback (most recent call last):
2009-08-06T03:24:30 SCRIPT_ERROR File "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Sift.py", line 83, in tick_operation
2009-08-06T03:24:30 SCRIPT_ERROR quality = 10 * self.get_quality(self_loc.coordinates, target, moisture)
2009-08-06T03:24:30 SCRIPT_ERROR File "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Sift.py", line 20, in get_quality
2009-08-06T03:24:30 SCRIPT_ERROR if not hasattr(target, location):
2009-08-06T03:24:30 SCRIPT_ERROR TypeError: hasattr(): attribute name must be string
2009-08-06T03:24:30 ERROR Script for "985" has reported an error processing a tick operation. This entity is probably now inactive.

The line should be:

    if not hasattr(target, 'location')

[Al Riddoch (alriddoch)]

Thanks for the updates.

Now I get this problem:

2009-08-06T14:19:23 ERROR Python error calling "tick_operation"
2009-08-06T14:19:23 SCRIPT_ERROR Traceback (most recent call last):
2009-08-06T14:19:23 SCRIPT_ERROR File "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Sift.py", line 80, in tick_operation
2009-08-06T14:19:23 SCRIPT_ERROR moisture = 10 * world.moisture
2009-08-06T14:19:23 SCRIPT_ERROR AttributeError: 'server.Entity' object has no attribute 'moisture'
2009-08-06T14:19:23 ERROR Script for "24" has reported an error processing a tick operation. This entity is probably now inactive.

[ameyp (ameyp)]

Fixed. The code for setting a moisture attribute for the world was present in the old branch, but I forgot to make those changes in the new branch.
I now get another error:

009-08-06T20:20:10 ERROR Python error calling "tick_operation"
2009-08-06T20:20:10 SCRIPT_ERROR Traceback (most recent call last):
2009-08-06T20:20:10 SCRIPT_ERROR File "/usr/local/share/cyphesis/rulesets/mason/world/tasks/Sift.py", line 83, in tick_operation
2009-08-06T20:20:10 SCRIPT_ERROR quality = 10 * self.get_quality(self_loc, target, moisture)
2009-08-06T20:20:10 SCRIPT_ERROR File "/usr/local/share/cyphesis/rulesets/mason/world/tasks/Sift.py", line 22, in get_quality
2009-08-06T20:20:10 SCRIPT_ERROR normal = target.location.parent.terrain.get_normal(location);
2009-08-06T20:20:10 SCRIPT_ERROR TypeError: function takes exactly 2 arguments (1 given)
2009-08-06T20:20:10 ERROR Script for "183854" has reported an error processing a tick operation. This entity is probably now inactive.

[Al Riddoch (alriddoch)]

The change you pushed has not fixed the problem. You cannot assume that the Weather entity is present, or that the world has a moisture value. You need to check using hasattr(), and use a default value if it does not have one.

The problem with your call to get_normal is that you are passing in location, but you need to pass in two arguments, an x and y coordinate. I am surprised you are reporting this, as you wrote the get_normal binding.

[ameyp (ameyp)]

I'd written the original binding to take the x and y coordinates. In
the new binding, I have declared the function as

static PyObject * TerrainProperty_getNormal(PyTerrainProperty * self,
PyObject * args)

and I am getting the x and y coordinates as

float x,y;
    if (!PyArg_ParseTuple(args, "ff", &x, &y)) {
        return NULL;
    }

The way I understood this was that if I pass the coordinates to this
function, the above code will parse that tuple and extract the x and y
coordinates from it. Is that incorrect?

Sorry about the moisture part, I assumed that the weather entity would
be present and have put the check for moisture inside Weather.py. Have
made changes in Sift.py and committed.

[Al Riddoch (alriddoch)]

Looking through the git logs for this file, I am not seeing different versions of the binding.

All python binding functions that take multiple arguments receive those arguments as a type, which they parse using the PyArg_ParseTuple() function. This binding now, as originally written, needs to be passed two arguments, the x and y coordinate of the point on the terrain where you want to know the normal.

Even if it was able to decode a tuple, the coordinates object you pass to this function is not a tuple. It is a physics.Point3D object, so a tuple decoding function would not be able to extract values from it.

In general any script in the server should be robust enough to run without errors no matter what else exists or does not exist in the world. Whenever we come across situations where a script throws an exception, we should fix the script rather than modifying the game world around them.

[Al Riddoch (alriddoch)]

I came across a few issues with fishing related tasks, and made a couple of changes which are in the attached patch.

The first change fixes a bug in the terrain property binding which did not create the vector object correctly. This is a very easy one to miss.

The second changes fixes a couple of issues in Sift.py, firstly passing the correct arguments to the terrain.get_normal() binding, and secondly fixing the call to normal.mag()

The sift task now works for me, but I am having a few issues with the Fishing goal still.

[ameyp (ameyp)]

I was getting another error in Sift related to the quality being a float, have fixed that by explicitly casting it to int.
The fishing task is not getting initiated even upon clicking on "sow with fishingrod", am looking into that.

[ameyp (ameyp)]

The task now initiates properly, but while attempting to move the bait to below the float, the following errors are given

2009-08-10T00:49:51 WARNING Broadcasting move op from 192146
2009-08-10T00:49:51 NOTICE ERROR generated by 190669 with message [Move op does not have correct id in argument]

The second error repeats itself several times with incrementing IDs.

[Al Riddoch (alriddoch)]

You need to set TO on the move operation, or it gets broadcast to every entity in the server.

         res = Operation("create", Entity(name = "float", parents = ["float"], location = float_loc), to = target)
- res = res + Operation("move", Entity(bait.id, location = bait_loc))
+ res = res + Operation("move", Entity(bait.id, location = bait_loc), to = bait)
         res = res + Operation("create", Entity(name = "hook", parents = ["hook"], location = bait_loc), to = bait)
         return res

I am currently working through other issues, as I still seem to have difficulty getting this task to work.

[ameyp (ameyp)]

Could you tell me what other issues you are facing?

On Tue, Aug 11, 2009 at 7:16 PM, Al Riddoch<email address hidden> wrote:
> You need to set TO on the move operation, or it gets broadcast to every
> entity in the server.
>
>
>         res = Operation("create", Entity(name = "float", parents = ["float"], location = float_loc), to = target)
> -        res = res + Operation("move", Entity(bait.id, location = bait_loc))
> +        res = res + Operation("move", Entity(bait.id, location = bait_loc), to = bait)
>         res = res + Operation("create", Entity(name = "hook", parents = ["hook"], location = bait_loc), to = bait)
>         return res
>
> I am currently working through other issues, as I still seem to have
> difficulty getting this task to work.
>
> --
> Fishing script is broken
> https://bugs.launchpad.net/bugs/409654
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in Cyphesis, the WorldForge game server: New
>
> Bug description:
> I get this error when initiating a fishing task:
>
> 2009-08-06T03:16:03 SCRIPT No bait in inventory
> 2009-08-06T03:16:03 ERROR Python error calling "tick_operation"
> 2009-08-06T03:16:03 SCRIPT_ERROR Traceback (most recent call last):
> 2009-08-06T03:16:03 SCRIPT_ERROR   File "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Fishing.py", line 82, in tick_operation
> 2009-08-06T03:16:03 SCRIPT_ERROR     for item in bait.contains:
> 2009-08-06T03:16:03 SCRIPT_ERROR NameError: global name 'bait' is not defined
> 2009-08-06T03:16:03 ERROR Script for "985" has reported an error processing a tick operation. This entity is probably now inactive.
> 2009-08-06T03:16:19 ERROR Connection(983) has found a character in its dictionery which is not connected.
>

[Al Riddoch (alriddoch)]

I am trying to get float creation working using the coordinates that the player clicks on, and make sure the hook creation is working in a more sane way. I'll explain in more detail when I have time away from work to look deeper into the code.

Can you confirm whether using the shovel works correctly for you? When you click on the terrain to activate the task, does the pile of earth appear in the same place?

[ameyp (ameyp)]

I can't verify whether the pile is being created at the exact same
coordinates as where the player clicks. I understand that Erik has
implemented a new command called /show_serverLogger that shows all the
traffic that is being sent to the server, but since I cannot update
the repo from here, I cannot test it out. However, visually, the pile
seems to be created at the same place where the player clicks.

[Al Riddoch (alriddoch)]

If you see the pile of earth even roughly near where you clicked then it is working correctly. If the pile is appearing somewhere strange, nowhere near then there is some kind of bug which we will need to track. Both erik and I have no problem with the coordinates for task activation, so if you can really confirm that it doesn't work for you, we will need details.

[Al Riddoch (alriddoch)]

I tried to attach this patch earlier, but it seems to have gone missing. It does two things:

a) It uses the position the user clicked on, passed in at activation as the position for the "float".

b) It massively simplifies setting the location of the bait.

I note that your tick_operation() handler never returns a new tick operation, so will only ever get called once. Generally a tick_operation handler should either return a new tick operation by calling self.next_tick(), or call self.irrelevant() to mark the task as obsolete so it can be terminated. This is also an issue with the Sift task, which when it completes does not call self.irelevant(), so it remains the active task, but never ticks again.

[ameyp (ameyp)]

I just experimented and realized that the float can be created where
the user clicks, the way it happens with the Dig task, and pushed code
to that effect and then saw this mail. I've made another commit for
adding self.next_tick() to Fishing and self.irrelevant() to Sift.

[Al Riddoch (alriddoch)]

Ok, great.

A few observations about your changes and your code:

You have to call .copy() when you want to copy a location:

    bait_loc = float_loc.copy()

You don't need to set the velocity of float_loc. This is a new Location object, not a copy, so the velocity is disabled by default.

self.irrelevant() does not return anything, so you do not need to append what it returns to res.

[Al Riddoch (alriddoch)]

Some progress:

The fish AI had been disabled some time ago, probably because they did nothing. The attached patch re-enables it, and once you have this change, you should start getting debug info from your fish AI.

[Al Riddoch (alriddoch)]

I am definitely now seeing active fish AI. They are moving around in the water, and running away from me when I get close. They also move out of the water, because the physics system doesn't really stop them yet. Don't worry about this aspect.

I am also seeing errors from some of the AI code. Disable the herd code, as that will only cause problems, and see how you get on with with AI you need for fishing.

[ameyp (ameyp)]

I've already disabled the school goal. You've merged those changes in
your fishing branch as well, I saw a commit.
The problem with testing it right now is that since there are no
restrictions on where the fish can move, it'll be hard for players to
pick a location at which to fish, which is why I was suggesting
setting up a few locations that will act as magnets for fish, ensuring
that a large density of fish will always be present at those
locations. Should we implement that now?

Amey

[Al Riddoch (alriddoch)]

Top priority right now is getting the code to the point where we can both try and fishing, and it works well enough that we can actually catch a fish. There are a huge number of options for sensibly working out how to constrain where the fish are, and I think it best you leave it at low priority for now. If you have time after working out how to catch fish, I would suggest then seeing if you can get the boat working.

Al

[ameyp (ameyp)]

It's precisely for testing the process of catching a fish that I want
to constrain the fish to a particular region. If we don't know where
the fish will be at a given point of time, how will we repeatedly test
the fishing task and remove all the bugs?

[Al Riddoch (alriddoch)]

You could add a goal to constrain their location, or you could remove the amble goal completely while you are testing the fishing goal code.

For my testing, I generated a cluster of fish in the shallow water around (-30,-30,0) where I can see the fish easily, and try out the goal there.

Al

[ameyp (ameyp)]

Ok, and are the fish getting caught and added to your inventory? If
not, is cyphesis giving any errors?

[Al Riddoch (alriddoch)]

I hadn't got that far yet, but was working on debugging the fish AI.

I should have some time to look at the code tomorrow, but with any luck you'll have it working by then.

[ameyp (ameyp)]

Ok, I think I misunderstood what the Forage goal does. It does not
seem to dynamically update when new targets are added. I placed debug
print statements inside the Forage goal, and inside spot_something and
pick_up_something, which are called by forage. The print statements
for forage and spot_something are called as soon as I log in to the
world, but if I create a new annelid near the fish, then the print
statements are not called again. Also, I have created forage goals for
"scrawny earthworm", "juicy earthworm" and so on, and all the print
statements are called with these targets at the beginning itself,
despite the fact that those entities are not present in the world at
that time. Is there any hunting goal which is dynamically called
whenever new entities that belong to the target type are created?

Amey

[Al Riddoch (alriddoch)]

I have made some more progress.

I sent you a mail yesterday about Forage. Forage does update when new targets are added, but only when the current target has been eaten.

Forage works on types, not on name, so at the moment you have to use "forage('annelid')" rather than using the other names you were using.

You have added a very small bbox to the annelid type. While this is accurate, it is so small that the fish can't even see them. I made the box ten times as large on each axis, and now the fish can see them. If you want something to be less than 10cm per side, it is best to have no bbox at all, and it will have a reasonable default visibility.

I have fixed a bug in the getNormal code which would cause an assertion failure if it was called when eieveing a pile of earth in anywhere but a small area of the server.

[Al Riddoch (alriddoch)]

I have push my fix for TerrainProperty::getNormal() in C++ on master.

Do you have enough information to fix setting up the forage goal with "annelid" rather than "juicy earthworm", and either make the bbox of annelid bigger, or remove it completely?

[ameyp (ameyp)]

Yes, I'll make those changes and push

[Al Riddoch (alriddoch)]

Another fix:

This one ensures the hook really is created inside the bait, not just at the same position. Do you follow the difference?

With this patch in place, I have watched a shoal of fish converge on the bait, and I have seen the bait disappear. I then confirmed using cycmd that the hook was inside the bait, and the bait was inside a fish, but for some reason the fish did not eat the bait, destroying it.

[Al Riddoch (alriddoch)]

Now that I have started debugging Fishing.tick_operation, I have discovered all sorts of things wrong with it:

1) You try/except block is hiding all the errors from you.

2) "bait" is uninitialised in tick_operation. You need to store then ID of the bait entity when the task is initalised with something like this:

    self.bait_id = bait.id

then in the tick_operation function, you can get the actual bait object with:

    bait=server.world.get_object(bait.id)

The first time tick_operation is called is the first opportunity for you to discover the ID of the hook entity, so you need to determine that, and store it for future use. Next time tick_operation is called, then bait entity may no longer exist, as it gets destroyed when it gets eaten.

3) You cannot have a long term loop in a task method, as it will block everything in the whole server. The server is a single thread of execution. You have to do all the checks you can, then return a tick to be called again, using next_tick(). Once you have done the setup, which included discovering the hook ID, all that needs to be done in each call to tick_operation is to check whether the hook is inside the bait or not. As soon as it is no longer in the bait, you can then determine what it is inside, and that is whatever has been caught.

Is all this clear?

[ameyp (ameyp)]

Yes, I realize the difference. I wanted to create the hook inside the
bait, and I assumed that creating it at the same location would
achieve that. But how does this code ensure that it is created inside
the bait and not just at the same location?

I put the try-except block in place because I was getting an error
related to the bait variable being uninitialized. I thought that the
tick operation was being called before the sow operation, so I put the
try-except there to just call tick again. Thanks for clearing that up,
I did not realize that external variables initialized in sow would not
be available in tick.
I'll save the hook id too by the same method, and push the changes in a bit.

[Al Riddoch (alriddoch)]

If you look at the patch I attached, the Location given to the move operation uses bait as the parent. When the location of entity has another entity as the parent, it is considered to be inside that entity, and the coordinates are then relative to that entities position. This is very similar to the way the character is moved into the boat, as we discussed elsewhere.

Looking forward to trying out your new fishing code soon.

[ameyp (ameyp)]

I've pushed all the changes.
How does that set the parent as bait? Doesn't Location(bait,
Point3D(0,0,0)) simply mean "Take the location of bait, and relative
coordinates as (0,0,0) and proceed"?

Thanks,
Amey

[Al Riddoch (alriddoch)]

Ok, the code looks better, but still has issues:

Once the bait has been eaten, server.world.get_object() will return None for bait, so bait.id won't work. But you don't need to check for bait.id, as you already have self.bait_id.

Just to be safe you should also check if bait is not None before the loop which iterates over bait.contains, and also check both hook_id and hook, as those calls are not guaranteed to succeed.

With the latest code you just pushed, the Fishing task crashed, but I was able to confirm that the hook is object is inside a fish now, and the bait was eaten. What happened for you when you tried the code? Did you get the task crashing in the same place?

[Al Riddoch (alriddoch)]

The first argument of Location constructor is the parent, so passing in bait as that argument makes the location inside the bait.

[Al Riddoch (alriddoch)]

One more observation:

At the end of the task you try and add the fish to the contains of the character directly. You cannot directly manipulate contains like this. You must move the entity with Move operation, just as you did with the bait when you move it into the water.

    fish = hook.location.parent
    Operation("move", Entity(fish.id, Location(self.character, Point3D(0,0,0)), to=fish)

[ameyp (ameyp)]

No, the task was not crashing. Everything seemed to go well, and the
progress bar was displayed. I'll make these changes and check again.

[ameyp (ameyp)]

And if the the call to server.world.get_object(self.bait_id) returns
None, then that means that the bait no longer exists. So can I go
ahead and assume that this is because a fish has eaten it, or is there
anything else that can happen that might cause the bait to get
destroyed?

Amey

[Al Riddoch (alriddoch)]

2009/8/16 ameyp <email address hidden>:
> And if the the call to server.world.get_object(self.bait_id) returns
> None, then that means that the bait no longer exists. So can I go
> ahead and assume that this is because a fish has eaten it, or is there
> anything else that can happen that might cause the bait to get
> destroyed?
>

It is probably going to be true most of the time, but if the bait does
get destroyed for some reason, then the parent of the hook will be the
world, so you should add a double check that the hook is inside
something sane. The double check is not necessary for now though.

Al
--
Alistair Riddoch
<email address hidden>
http://alistairriddoch.org/

[Al Riddoch (alriddoch)]

One final fix to your latest push, and now it works.

You had the arguments to the move operation wrong.

[ameyp (ameyp)]

Pushed.
So everything is working now? Is the bait getting destroyed?

[Al Riddoch (alriddoch)]

The basics are now working. The bait gets destroyed, and the fish is placed in the characters inventory.

The are a few lingering issues with some of the error checks.

If you detect an error which means the task cannot continue, you should call self.irrelevant() and then return. Just returning means the task stays there, but is forever stalled. Just calling irrelevant() without returning means the rest of the code will try to run, even though the task no longer serves any purpose.

[Al Riddoch (alriddoch)]

One final thing you might like to consider would be cleaning up the float entity once the task is complete, or aborted.

[ameyp (ameyp)]

How should I obtain the id of the float entity? It is not contained in
anything, nor does it contain anything.
I've added the necessary return and self.irrelevant() statements at
various places and pushed.

[Al Riddoch (alriddoch)]

Good point. In that case the only option is to use the transient property, which you can add to the XML file, and looks like this:

      <map name="transient">
        <float name="default">1800</float>
        <string name="visibility">public</string>
      </map>

This specifies that the entity does not exist permanently, and after a number of seconds it should be automatically deleted.

[Al Riddoch (alriddoch)]

There has been no push from you since the one where you fixed the move operation. Either you forgot, or github has somehow lost it.

[ameyp (ameyp)]

I'm sorry, I guess I forgot. I've also added the transient property to
the float entity, and pushed all changes.

On 8/17/09, Al Riddoch <email address hidden> wrote:
> There has been no push from you since the one where you fixed the move
> operation. Either you forgot, or github has somehow lost it.
>
> --
> Fishing script is broken
> https://bugs.launchpad.net/bugs/409654
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in Cyphesis, the WorldForge game server: In Progress
>
> Bug description:
> I get this error when initiating a fishing task:
>
> 2009-08-06T03:16:03 SCRIPT No bait in inventory
> 2009-08-06T03:16:03 ERROR Python error calling "tick_operation"
> 2009-08-06T03:16:03 SCRIPT_ERROR Traceback (most recent call last):
> 2009-08-06T03:16:03 SCRIPT_ERROR File
> "/opt/worldforge/share/cyphesis/rulesets/mason/world/tasks/Fishing.py", line
> 82, in tick_operation
> 2009-08-06T03:16:03 SCRIPT_ERROR for item in bait.contains:
> 2009-08-06T03:16:03 SCRIPT_ERROR NameError: global name 'bait' is not
> defined
> 2009-08-06T03:16:03 ERROR Script for "985" has reported an error processing
> a tick operation. This entity is probably now inactive.
> 2009-08-06T03:16:19 ERROR Connection(983) has found a character in its
> dictionery which is not connected.
>