skell
Level: Legendary Smitemaster

Rank Points: 3734
Registered: 12-28-2004
IP: Logged

So apparently the splitting logic was indeed there, though I reimplemented it myself because I had broken tests and thought it isn't there.

Anyway, built doors will now merge to avoid duplicate connections to the same orb/plate, and any conflicting actions will be resolved to toggle.

PR

This is the last thing I had in store for 5.1.1, time to try to get the Linux build to, erm, build.

____________________________
My website | Facebook | Twitter

kieranmillar
Level: Smitemaster
Rank Points: 2670
Registered: 07-11-2014
IP: Logged

Just tested this in 5.1.1.alpha.2020-10-29 and I don't think it's working as intended.

Scenario:

There are two doors, both controlled by the same orb. A build command merges these two doors into one. End result: The orb maintains two different connections to the same door, and its effects stack. So for example, if the orb toggled both doors, now when you strike the orb, it will toggle the door twice, so effectively nothing happens.

skell
Level: Legendary Smitemaster

Rank Points: 3734
Registered: 12-28-2004
IP: Logged

The status says it's still waiting for merge so I don't think it made it into the build yet

____________________________
My website | Facebook | Twitter

kieranmillar
Level: Smitemaster
Rank Points: 2670
Registered: 07-11-2014
IP: Logged

Oh yeah, of course, I should have guessed a change made on the 30th would be in the 29th alpha build.

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

Couple of observations about the algorithm you've used to merge the doors:


1. As it stands now, the code will always run a merge if at least one agent is found. That's unnecessary. It'd be better to keep a running count of all agents found pointing to the doorCoords, and do the merge only if it's greater than 1.


2. You don't need to collect boolean information about bHasOpen/bHasClose/bHasToggle. You can start with 'OrbAgentType targetAction = OA_NULL' and just change that as you iterate over each agent (since we've chosen a solution where order doesn't matter). The code for that would look something like this:

   agentsFound++;

   if (pAgentData->action == OA_NULL)
      continue;  // Really shouldn't happen... might be worth an assert as well?

   if (targetAction == OA_NULL)
      targetAction = pAgentData->action;
   else if (targetAction != pAgentData->action)
      targetAction = OA_TOGGLE;

Feels like there could be a way to reduce the routine to one loop through the agents (followed by an editing of the first Agent found). The first iteration could also be ended early if targetAction has become OA_TOGGLE and agentsFound >= 2.

But both of those suggestions are probably a bit more involved for not much gain. Definitely think avoiding a second iteration if there's only one agent at all is worthwhile though.


Other than that, the whole thing looks a lot neater than I probably would've done it -- was always more comfortable editing old code than creating new. Nice work!

kieranmillar
Level: Smitemaster
Rank Points: 2670
Registered: 07-11-2014
IP: Logged

kieranmillar wrote:
Just tested this in 5.1.1.alpha.2020-10-29 and I don't think it's working as intended.

Scenario:

There are two doors, both controlled by the same orb. A build command merges these two doors into one. End result: The orb maintains two different connections to the same door, and its effects stack. So for example, if the orb toggled both doors, now when you strike the orb, it will toggle the door twice, so effectively nothing happens.

This is still happening in 5.1.1.alpha.2020-11-04 for what it's worth, I'm not seeing any changes. Maybe it's not in this alpha either?

skell
Level: Legendary Smitemaster

Rank Points: 3734
Registered: 12-28-2004
IP: Logged

File: BUG REPORT Merging yellow doors.hold (1.1 KB)
Downloaded 42 times.
License: Public Domain

@kieranmillar - Seems to work fine for me. I've downloaded 5.1.1.alpha.2020-11-04 from this post and tested the attached hold, and didn't get multiple connections. Can you please share the hold you used for testing?

____________________________
My website | Facebook | Twitter

kieranmillar
Level: Smitemaster
Rank Points: 2670
Registered: 07-11-2014
IP: Logged

File: BUG REPORT Multi orb connections.hold (1 KB)
Downloaded 42 times.
License: Public Domain

See attached hold. Your hold works for me, but this hold does not.

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

Oh my, that's a pretty fantastic bug. I mean, not the bug itself, but just the fact that skell's test cast just *happens* to evade it.

It's due to the new code in MergeYellowDoorConnectionsInArea. It's supposed to test all coordinates from (wX,wY)-(endX,endY).

Unfortunately, it's currently testing (wY,wX)-(endX,endY).

So a door built at (12,12) works fine. But one built from (16,11)-(18,13) ends up testing (11,16)-(18,16), and so ends up not testing any of the doors in that room at all. Simple mistake, but it's amazing that the original test case just happens to pick one of the few ways it'd actually work properly.

skell
Level: Legendary Smitemaster

Rank Points: 3734
Registered: 12-28-2004
IP: Logged

I am not even mad that's amazing

Edit: PR

@TFM: I also included your optimization suggestions. There was another issue - it would add the same orb multiple times to the vector and merge the orb once for each agent.

____________________________
My website | Facebook | Twitter

---

[Last edited by skell at 11-08-2020 04:27 PM]

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

skell wrote:
@TFM: I also included your optimization suggestions. There was another issue - it would add the same orb multiple times to the vector and merge the orb once for each agent.

Hmm... well, the main optimization issue you're running into is that you're iterating over the agents far too many times. EDIT: To be clear here, I'd just been concentrating on MergeOrbConnections when I was looking at the code originally, so I thought you were looking at each orb a maximum of twice. I hadn't realised there was already another iteration in GetOrbConnections, so I'm addressing that now in this post.

The code looks like it's doing something like this:

* GetOrbConnections: Iterate through all orbs in the room. Iterate through all agents in each orb. If agent connects to merged door, mark orb for merging. (With the PR, this now only marks an orb if 2 or more agents connect to merged door.)
* MergeOrbConnections Part 1: Iterate through all agents in a marked orb. Collect information about each action of the agent if it points to merged door. If at least one action is found, move to Part 2.
* MergeOrbConnections Part 2: Iterate through all agents in a marked orb. If it doesn't point to the merged door, ignore it. If it's the first action found that does so, edit the action to whatever was decided in Part 1; if it's not the first action, delete it.

So each orb is being iterated through at least once, and merged orbs are being looked through up to three times.

Instead, you could drop GetOrbConnections, and do something like this:

* MergeOrbConnections Part 1: Iterate through all agents in an orb (marked or not). Collect information about each action of the agent if it points to merged door. Ignore actions that don't point to merged door. If it's the first action found, keep track of it. If it's not the first action but still must be merged, delete it.
* MergeOrbConnections Part 2: Edit the first action found with the combined action we calculated in Part 1.

This way, each orb is iterated over once only, merged actions are removed immediately, and orbs that have no actions that need to be merged are already ignored.

---

[Last edited by TFMurphy at 11-08-2020 05:25 PM]

skell
Level: Legendary Smitemaster

Rank Points: 3734
Registered: 12-28-2004
IP: Logged

I'd rather keep it as is - there are no cases where it would make any discernable difference and changing it would likely reduce reafability.

____________________________
My website | Facebook | Twitter

kieranmillar
Level: Smitemaster
Rank Points: 2670
Registered: 07-11-2014
IP: Logged

Can confirm that this is now all working in 5.1.1.beta.2020-11-12. When different doors with commands from the same source are merged into one and they had different effects on the doors, they turn into toggles.