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

IStepper Enabled property does not work as described. #768

Open
laultman opened this issue Nov 1, 2023 · 8 comments
Open

IStepper Enabled property does not work as described. #768

laultman opened this issue Nov 1, 2023 · 8 comments

Comments

@laultman
Copy link

laultman commented Nov 1, 2023

Description

According to the documentation if class derived from IStepper has its Enabled property set to false the Step method on that class is not called (assuming thereby, time is saved or other state evaluations may be avoided in not checking anything in the Step method.

I subclassed IStepper into an abstract class that overrides the Enabled property giving it a setter which really just hides the IStepper Enabled property. Currently the IStepper class does not provide a setter and no method to set the Enabled property bool state.

Since there is no way to change the state of the Enabled property even in an override the Step method continues to be called. What is the recommended way of setting the Enabled property?

// from StereoKit Framework IStepper.cs file
	public interface IStepper
	{
		/// <summary>Is this IStepper enabled? When false, StereoKit will not
		/// call Step. This can be a good way to temporarily disable the
		/// IStepper without removing or shutting it down.</summary>
		bool Enabled { get; }

Is this IStepper enabled? When false, StereoKit will not call Step. This can be a good way to temporarily disable the IStepper without removing or shutting it down.

Platform / Environment

Windows 11 latest production build, VS2022 latest updates, StereoKit 0.3.8

Logs or exception details

There were no exception.
@laultman
Copy link
Author

laultman commented Nov 1, 2023

Looking at the SK.cs Step method, I don't see any checks of the Enabled property. I set up a small test case.

public class TestStepper : IStepper
{
  public TestStepper ()
  {
    Enabled = false;
  }
  public bool Enabled { get;}
  public bool Initialize()
  {
	return true;
  }
  public void Shutdown() {}
  public void Step()
  {
     var x = Enabled;
  }
}

In this test Enabled is in fact false. With a breakpoint set on the statement in the Step() method, the debugger does stop there indicating that Step() method was called even when Enabled is false.

@laultman
Copy link
Author

@maluoi Did you see this? Any comments on what to do with the Enabled flag. There must be some reason you did not allow the Enabled flag to be changed. I can't think of a use case where I create an object only to set it to not Enabled. Since there is no setter on the property it can't be set outside private code in the class. How did you intend for us to use this property?

@maluoi
Copy link
Collaborator

maluoi commented Nov 22, 2023

I have been thinking about this a bit lately. It's not currently used but I don't remember why, and it probably should be especially considering what the docs say. It's been a while since I wrote that!

However, here's how you should use it:

public class TestStepper : IStepper
{
    // If you just want it to always step
    public bool Enabled => true;
    // If you want to control stepping
    public bool Enabled { get; set; } = true;
    
    // The way it probably should work
    public void Step() {}
    
    // How you can do it properly today
    public void Step() {
        if (!Enabled) return;
    }

    public bool Initialize() => true;
    public void Shutdown  () {}
}

I suppose it's good to note explicitly here that the Enabled definition in the interface is merely the minimum requirement. It must have a public get, but the rest is up to you. It can have a public set, or a private set, the interface doesn't care.

@laultman
Copy link
Author

@maluoi I'm using it in today's current mode as you have in the response above. I am integrating Teams into my application and there are parts of Teams that need to be "on" all the time and the rest needs to be off when not in a call. I am using the whole concept of your Step() as a driving model in my designs for industrial applications. These are highly dynamic so anywhere I can save a CPU/GPU cycle or two is helpful.
My assumption is that if the main loop doesn't even call a Step() then I've saved some downstream workload. I might be totally off base here but it's logical. For one thing, I could take disabled Steppers out of my active dictionary that handles my system state. That immediately shortens my look-up list. I think that having it "really" disabled changes the way you think about the coding. It has for me, at least.
Hopefully the SK Stepper loop is highly optimized for its own internal operations.

@laultman
Copy link
Author

@maluoi Here is what I am currently doing. I have an interface, ImyInterface : IStepper. In ImyInterface I added a "new bool Enabled {get; set;}" definition to overrule the IStepper's Enabled property. Then I added a bunch of other properties and other method templates for ImyInterface as needed by my code.
I then created an abstract class to implement ImyInterface. I'm using an abstract class to add defined functionality to deriving class while not limiting future code from either using the abstract or rolling their own.

The million-dollar question: Does this really affect the StereoKitC code? I cannot find any definition in it where Enabled is used as a value that is evaluated by the C++? I think you answered it already, that it is not currently implemented, but just double-triple checking. C# hiding or overruling an interface in a concrete class must in the end be properly extended to the NativeAPI calls.

public interface ImyInterface : IStepper
{
   new bool Enabled { get; set; } // I really do want to create a new Enabled, overruling the IStepper's Enabled property.
   // the rest of the ImyInterface ...
}

// usage
public abstract class MyClass : ImyInterface
{
   public bool Enabled { get; set; } = false; // The Enabled property is a concrete implementation in this base class.
   // the rest of my abstract implementation ...
}

@maluoi
Copy link
Collaborator

maluoi commented Nov 22, 2023

The IStepper is a C# only construct, so it never shows up in the C codebase. Checking Enabled in SK's code rather than the IStepper's, or skipping it altogether shouldn't confer any notable performance differences, for me it's purely a design concern.

Your public interface ImyInterface : IStepper seems great, the IStepper is really only the minimum that StereoKit wants to tap into, so extending it for your own project is quite appropriate!

@laultman
Copy link
Author

@maluoi I do hope you will make the change to skip the Step() call when the IStepper is not enabled. I asked a couple questions over in the discussion section #786 and #787 if you could glance at those?

@laultman
Copy link
Author

For anyone encountering this thread. Currently (0.3.8) you must manage the Enabled property, which is simple but nice to know. The other two issue topics are also answered, if you wish to see them.

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

2 participants