Code review for first script

Hi all. I am relatively new to SE, and just getting started with the PG and C#, but not new to programming at all. I spent a bit of time this evening reading the docs and some of the posts in this forum (I will certainly read more of them, as there is a lot of good info here) and I worked out my first sort of test script. I value peer review and am mostly just interested in whether I have the basics down correctly or not. Can folks take a look at my script and point out any bad patterns I might have? One of my main concerns is whether I am doing things efficiently or not, as while this is a pretty simple script, I would like to work my way up to fancier things. C# tips are welcome too.

Thanks!

BTW the use-case for this script is just a survival world I've been playing with my young son. He's been having a bit of trouble getting the sensors to work right to trigger our airlocks and whatnot and I figure something like this will help. I'm sure there are scripts out there that do the same basic thing, but I want to do it myself.

Code:

/*
Usage:
Create groups of sensors and doors that you wish to automatically open and close.
All doors in the group will automatically open when any sensor detects an entity
within range. The groups must have the word "autodoor" in them. The groups can
have an arbitrary number of sensors or doors, with the simplest group being
a single door and a single sensor.

Copy/paste this script into a Program Block and run it.
Then go name the doors and sensors and add them to your "autodoor" groups.

Simplest Example:
* Door: "door"
* Sensor: "sensor"
* Group contains both of the above and is named "autodoor"

Example wth a bigger group:
* Doors: "door", "door2", "door3"
* Sensor: "sensor", "sensor2"
* Group contains all of the above and is named "lots of autodoors"
*/

const string autoGroupTag = "autodoor";

List<MyDetectedEntityInfo> entitiesDetected = new List<MyDetectedEntityInfo>();
List<IMyBlockGroup> autoGroups = new List<IMyBlockGroup>();
List<IMyDoor> doors = new List<IMyDoor>();
List<IMySensorBlock> sensors = new List<IMySensorBlock>();

int cacheRefreshCounter = 0;

public Program()
{
	Runtime.UpdateFrequency = UpdateFrequency.Update10;
	updateCache();
}

public void updateCache() {
//	Echo("cacheRefreshCounter: " + cacheRefreshCounter);
	if (cacheRefreshCounter % 20 == 0) { // this interval is sort of arbitrary ATM
		GridTerminalSystem.GetBlockGroups(autoGroups, group => group.Name.Contains(autoGroupTag));
	}
	cacheRefreshCounter++;
}

public void Main()
{
	updateCache();

	foreach (IMyBlockGroup group in autoGroups) {
		group.GetBlocksOfType(sensors);
		if (sensors.Count < 1) continue;

		group.GetBlocksOfType(doors);
		if (doors.Count < 1) continue;

		bool anythingDetected = false;
		foreach (IMySensorBlock sensor in sensors) {
			sensor.DetectedEntities(entitiesDetected);
			if (entitiesDetected.Count == 0) {
				anythingDetected = true;
				break;
			}
		}

		string action = anythingDetected ? "Open_Off" : "Open_On";
		foreach (IMyDoor door in doors) {
			door.ApplyAction(action);
		}
	}
}

 

Program():
See suggestion for updatecache()

updatecache():
The code assumes that it will update the cache on the first call. This assumption is not explicit.
I suggest making it explicit by adding

void updatecache(bool bForce=false)
...
if(bForce || cachReferenceCounter...


And call updatecache(true) from Program()

% 20 Move definition of this to above and use it instead of a 'magic' number embedded in the code.

This check also just counts the number of times it is run; not the actual time that has passed.
If the update is changed to Update1, it will run way to often.

Suggest changing to checking the actual time elapsed by adding up Runtime.TimeSinceLastRun and then comparing to the desired time delay. Since this value is not valid in constructor, you'll have to just check in Main() and also check for 'never run yet'. https://github.com/malware-dev/MDK-...me.IMyGridProgramRuntimeInfo.TimeSinceLastRun

Main()
You could cache the list of sensors and doors for each group to avoid getting them every run.

Use sensor.IsActive to detect if the sensor has detected anything. https://github.com/malware-dev/MDK-SE/wiki/Sandbox.ModAPI.Ingame.IMySensorBlock.IsActive

if(sensor.IsActive)
{
anythingDetected=true;
break;
}


Don't use actions when there are alternatives. https://github.com/malware-dev/MDK-SE/wiki/Do's-and-Don'ts

door.Enabled=false; will turn off a door. https://github.com/malware-dev/MDK-SE/wiki/Sandbox.ModAPI.Ingame.IMyFunctionalBlock.Enabled

so so you can replace the code with:
foreach (IMyDoor door in doors) {
door.Enabled=!anythingDetected;
}

 

@Wicorel, thank you for the feedback! This is pretty much exactly what I was looking for.

Great suggestion to express the cache update period in terms of "real time" (by accumulating Runtime.TimeSinceLastRun), I am not a fan of obscurity or fragility either. Also the "force cache update" pattern to handle startup is a nice tip.

Regarding caching each group of sensors/doors, I had considered this, but for whatever reason passed on it. Oh I recall, I was thinking in terms of reusing the memory allocated for the sensor and door lists. However I think I can do both... cache all blocks at once and mostly reuse the same memory... as long as the number of groups doesn't change between iterations.

Hey one thought about caching blocks in general - if there was a cheap way to tell if the grid had a block added or a name changed, then we could just update the cache only when it might need updating... I'm thinking push vs polling here... e.g. if the PG could register a callback so that the main SE engine could set a dirty bit when certain types of changes happen, then we'd be able to known when to try updating the cache. I guess I could do something like diff a hash of the grid's block names myself, but that would only make sense if it was cheaper than just redoing the lookups each time. Hmm, this is maybe getting overly complicated, but if updating the cache could possibly result in memory allocation, then checking for a diff could be cheaper. I'll just keep it simple for now and maybe file this away for use in a different problem.

The tip about just checking the IsActive property is great too. I was wondering if it was wasteful to use the DetectedEntities method there.

Thanks for the reminder to not use Actions when possible, this is all new to me so I am admittedly quite blind still. BTW my usage of DetectedEntities and ApplyAction were based on naively copying code from the example here:
https://spaceengineerswiki.com/Programming_Guide#Checking_a_sensor

Also, I assume that rather than:
door.Enabled =! anythingDetected;

You probably mean something more like:
if (anythingDetected) {
door.OpenDoor();
} else {
door.CloseDoor();
}
Right? As I want the door to open and close, not turn on and off. But your point is taken, there is a method I can use rather than an action.

Thanks again for the guidance!

 

There are no events for such things. It would be nice if there where. You can 'detect' grid changes by periodically checking the number of blocks on the grid; but to do so you have to get all the blocks.. You might as well just re-get everything every so often.

That wiki is not official and not being maintained.

Yes.. that's what I meant. Not turning on or off the door.

 

There is to some extent...
If you have a cached reference to a block you can quickly get a new, updated reference to it using method

Code:

IMyTerminalBlock IMyGridTerminalSystem.GetBlockWithId(long id);

So if you have

Code:

IMyTerminalBlock myCachedBlock;

you can write

Code:

IMyTerminalBlock theBlock = GridTerminalSystem.GetBlockWithId(myCachedBlock.EntityId);
if (theBlock == null)
{
  // the block has been removed from the grid, or is inaccessible, so the reference myCachedBlock is invalid
}
else
{
  // the block is still on the grid, so now we can check if theBlock.CustomName have different name than the last time
}

Each block have an unique id and the method GetBlockWithId performs quick lookup against that id in the internal dictionary, instead of enumerating each block on the grid and comparing name. And because of that this method it's pretty fast.

I haven't found a way to check if any block has been added to the grid since last run.

 

> IMyTerminalBlock theBlock = GridTerminalSystem.GetBlockWithId(myCachedBlock.EntityId);
> ...

Ok cool. So I can see this being a useful validation step, thanks for the tip, I will tuck it away for sometime when I might need it!

edit: words

 

For any future people who stumble down this thread, here is what I came up with in the end (based on the feedback here) and am currently using:
https://github.com/aortez/hydrogen-candy/blob/master/proximity_door_groups_efficient.cs