I appreciate your persistence which ultimately helped narrow this problem down.
Announcement
Collapse
No announcement yet.
Partner 728x90
Collapse
NinjaTrader
Why is State.Terminated() run when another, different indicator is loaded?
Collapse
X
-
Originally posted by NinjaTrader_Matthew View Post
I appreciate your persistence which ultimately helped narrow this problem down.MatthewNinjaTrader Product Management
-
The specific scenario we identified and will be resolved in the next build was:
- Custom indicator already is on chart (instance #1)
- Open indicator dialog -> instance #2 (clone of #1 above) and instance #3 (in the list of indicator names) are created + .SetDefaults is called -> good
- Remove the Custom indicator from the list of “Configured” indicators and press OK
-> Only 2 instance .Terminated are called and not 3 -> This was a bug which did not finalize the indicator until the next time you open/closed the list, which I believe explains some of the delayed behavior as a sub note to your report.
However, so I do not mislead anyone into thinking otherwise: State.Terminated is designed to, and will continue to be called at the time any instance of an indicator is being terminated.
Why are members of an object being changed based on actions on another object? Specifically, why is the exit routine of a class being run because of actions outside the class: actions that have nothing to do with the first class?"
So that results in your blue text i.e.,
BarMarginRightAtEntry: 0
CurrentBarMargin: 0
Since your private field defaults to 0, you checked only if ChartControl was not null and then tried to reset the value to a value that was still 0.
As such, you should take steps to make sure that the indicator instance was the actual indicator to modify a global property, and if so, only then undo those changes. In your example, you could check if that value was greater than 0 - or keep it agnostic of user preferences, it can more easily be accomplished simply by setting a flag during State.DataLoaded in your example, and then checking against that flag in terminated.
Code:private bool shouldResetBarMargin = false; private int _intBarMarginRightAtEntry = 0; protected override void OnStateChange() { if (State == State.DataLoaded) { if (ChartControl != null) { this._intBarMarginRightAtEntry = ChartControl.Properties.BarMarginRight; ChartControl.Properties.BarMarginRight = Math.Max(this._intBarMarginRightAtEntry, 260); shouldResetBarMargin = true; } } else if (State == State.Terminated) { if (shouldResetBarMargin) { if(ChartControl != null) { ChartControl.Properties.BarMarginRight = this._intBarMarginRightAtEntry; shouldResetBarMargin = false; } } } }
Opening or closing a dialog box should not cause exit processing in anything unrelated to the target of the invocation of said databox. Most certainly, I cannot see why exit processing is run when I merely open a dialog and close it without doing anything.
I understand you may not feel the same and were comfortable with the NT7 approach which only terminated indicators after they were running on the chart, but after careful review, we conclude that setting State.Terminated and disposing of all if its resources is a best case approach to ensure the finalization of every instance of that indicator.MatthewNinjaTrader Product Management
Comment
-
Originally posted by NinjaTrader_Matthew View PostThe specific scenario we identified and will be resolved in the next build was:
- Custom indicator already is on chart (instance #1)
- Open indicator dialog -> instance #2 (clone of #1 above) and instance #3 (in the list of indicator names) are created + .SetDefaults is called -> good
- Remove the Custom indicator from the list of “Configured” indicators and press OK
-> Only 2 instance .Terminated are called and not 3 -> This was a bug which did not finalize the indicator until the next time you open/closed the list, which I believe explains some of the delayed behavior as a sub note to your report.
However, so I do not mislead anyone into thinking otherwise: State.Terminated is designed to, and will continue to be called at the time any instance of an indicator is being terminated.
Per your scenarios, ChartControl is a global property and can be accessed by any other indicator, as well as other instances of the same indicator you're working with. In particular, the UI's "Configured" indicator instances was cloned from the running instance on the chart, so ChartControl would not have been null at the time the "Configured" instance is terminated from the UI. This caused an unrelated UI instance of the indicator to attempt to modify values which it did not modify to begin with.
So that results in your blue text i.e.,
BarMarginRightAtEntry: 0
CurrentBarMargin: 0
Since your private field defaults to 0, you checked only if ChartControl was not null and then tried to reset the value to a value that was still 0.
As such, you should take steps to make sure that the indicator instance was the actual indicator to modify a global property, and if so, only then undo those changes. In your example, you could check if that value was greater than 0 - or keep it agnostic of user preferences, it can more easily be accomplished simply by setting a flag during State.DataLoaded in your example, and then checking against that flag in terminated.
Code:private bool shouldResetBarMargin = false; private int _intBarMarginRightAtEntry = 0; protected override void OnStateChange() { if (State == State.DataLoaded) { if (ChartControl != null) { this._intBarMarginRightAtEntry = ChartControl.Properties.BarMarginRight; ChartControl.Properties.BarMarginRight = Math.Max(this._intBarMarginRightAtEntry, 260); shouldResetBarMargin = true; } } else if (State == State.Terminated) { if (shouldResetBarMargin) { if(ChartControl != null) { ChartControl.Properties.BarMarginRight = this._intBarMarginRightAtEntry; shouldResetBarMargin = false; } } } }
Agreed, your indicator should not have been running its exit processing at this time, and this can be managed by changing the indicators terminated logic to only process exit code of which it actually needed to process.
I understand you may not feel the same and were comfortable with the NT7 approach which only terminated indicators after they were running on the chart, but after careful review, we conclude that setting State.Terminated and disposing of all if its resources is a best case approach to ensure the finalization of every instance of that indicator.
The solution you propose is pretty much what Edge, NMJ and I, working together over this weekend, managed to figure out as what we considered to be a possible, kludgy, hacky, workaround.
Apparently, you collectively think that that is a solution, rather than a workaround. If that is the solution, even granted that maybe not many developers are using State.Terminated, there are still a considerable number of developers who understand the need to clean up in order to prevent resource leaks. As such, if this is the solution, then I shall have to request that it be officially documented. Further, it should actually be a part of the wizards, and so be placed as part of the skeleton whenever a strategy or indicator is built from the wizard.
In the absence of these, I shudder to think how many times your support is going to have to answer this question from developers who are not aware of this nuance. Is "nuance" even a valid description of this?
Just my $0.02.
Comment
-
Originally posted by koganam View PostWhereas I do not agree with your design decision, we must also acknowledge that it is your product, and you must be the final arbiter of what will be done.
The solution you propose is pretty much what Edge, NMJ and I, working together over this weekend, managed to figure out as what we considered to be a possible, kludgy, hacky, workaround.
Apparently, you collectively think that that is a solution, rather than a workaround. If that is the solution, even granted that maybe not many developers are using State.Terminated, there are still a considerable number of developers who understand the need to clean up in order to prevent resource leaks. As such, if this is the solution, then I shall have to request that it be officially documented. Further, it should actually be a part of the wizards, and so be placed as part of the skeleton whenever a strategy or indicator is built from the wizard.
In the absence of these, I shudder to think how many times your support is going to have to answer this question from developers who are not aware of this nuance. Is "nuance" even a valid description of this?
Just my $0.02.
Anything we could do here Product/Feature wise would only plug a concept hole that a developer have not prepared themselves for, but we will do our best to make the documentation clearer here in this regard.
Thank you.Last edited by NinjaTrader_Matthew; 03-24-2016, 07:31 PM.MatthewNinjaTrader Product Management
Comment
-
Originally posted by NinjaTrader_Matthew View PostIt should be documented and common practice that only a resource that has been set up/modified needs to be taken down/reset.
Anything we could do here Product/Feature wise would only plug a concept hole that a developer have not prepared themselves for, but we will do our best to make the documentation clearer here in this regard. Thank you.
All I am asking is that you reduce your support load by telling everyone that you have coded this violation, and how to deal with it. In other words, warn people that if they do not take special measures, such as you have explained here, to track what has been modified, and not just look at the object itself, their cleanup code will run at times that they do not expect; as objects that have not been applied to a chart can attempt to be terminated, or objects that are already on the chart will attempt to run cleanup terminal code, if you attempt to react with the system by even preparing to load another object.
Comment
-
Originally posted by koganam View Post
What "concept hole". That, in violation of the most basic concept of OOP, objects that are indicators or strategies in NinjaTrader can be modified by side-effects outside of the code in the object? That to me is a violation of OOP concepts. I guess maybe I do have a concept hole then, or maybe I, myself am the concept hole.
All I am asking is that you reduce your support load by telling everyone that you have coded this violation, and how to deal with it. In other words, warn people that if they do not take special measures, such as you have explained here, to track what has been modified, and not just look at the object itself, their cleanup code will run at times that they do not expect; as objects that have not been applied to a chart can attempt to be terminated, or objects that are already on the chart will attempt to run cleanup terminal code, if you attempt to react with the system by even preparing to load another object.
For example, if we provided a HasSeenHistoricalState bool, it could definitely help as you would not need to implement this yourself - however you would still need to check for it through code and would still require a bit of education as to why it needs to be implemented, and the current proposed solution is fairly trivial for a developer to implement.
I have marked on our list with greater priority to work on improving the help documents around lifetime management of an indicator, from setup to termination, and hope to provide more sound documentation to make users aware of this design pattern and the solution proposed in my prior post.
We are not currently in any state where we can change the way indicators state changes, but will consider this scenario for future iterations of NinjaTrader 8.MatthewNinjaTrader Product Management
Comment
Latest Posts
Collapse
Topics | Statistics | Last Post | ||
---|---|---|---|---|
Started by martini, Yesterday, 04:45 PM
|
1 response
9 views
0 likes
|
Last Post Today, 06:45 AM | ||
Started by proptradingshop, 03-21-2024, 09:50 AM
|
5 responses
26 views
0 likes
|
Last Post Today, 05:17 AM | ||
Started by MaupinFinche, Today, 05:08 AM
|
0 responses
5 views
0 likes
|
Last Post
by MaupinFinche
Today, 05:08 AM
|
||
Started by Enkidu, Yesterday, 06:40 AM
|
9 responses
35 views
0 likes
|
Last Post
by Enkidu
Yesterday, 01:11 PM
|
||
Started by Mestor, 03-10-2023, 01:50 AM
|
12 responses
310 views
0 likes
|
Last Post
by z.franck
Today, 04:29 AM
|
Comment