search

tom-r / tactics_pi

a performance enhancement of dashboard_pi for OpenCPN
16 stars 14 forks source link

Rudder dial instrument bug #48

Closed rgleason closed 4 years ago

rgleason commented 4 years ago

https://github.com/canne/dashboard_tactics_pi/issues/94

See this where it is fixed. https://github.com/canne/dashboard_tactics_pi/issues/94

ghost commented 4 years ago

Rick, thanks for this memo! You may or may not want to close this ticket: since there are no timestamps moving along the data in Tactics v1.0.11, I am quite sure that the issue with the dial base class taking over the data will not occur here. It is a good reminder for the developers that if timestamps are ever implemented, better check this class as well. But it is not a bug of Tactics v1.0.11, nobody has reported it.

rgleason commented 4 years ago

This is weird, I tried testing Tactics and it too seems to have the rudder the wrong way. Later I will post some series of creenshots using at this turn using sample2.txt from Tactics wiki page Don't understand this really. Got to go. Screenshot (282)

.

ghost commented 4 years ago

Oh, indeed that's weird! Maybe the issue is the same even if the root cause (timestamps in my case) is different: because of argument mismatch, the override fails in linking phase and the dial base class' SetData() gets called. But there is no sign inversion in that method.

Better keep this then open until we are sure.

On Friday, November 22, 2019, Rick Gleason wrote:

This is weird, I tried testing Tactics and it too seems to have the rudder the wrong way. Later I will post some series of creenshots using at this turn using sample2.txt from Tactics wiki page Don't understand this really. Screenshot (282)

..

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tom-r/tactics_pi/issues/48#issuecomment-55761165

-- Best regards / Cordialement / Parhain terveisin Petri Mäkijärvi

ghost commented 4 years ago

Rick, you can safely close this issue. You must be testing with DashT (with bug) and not with Tactics v1.0.11: Remember when I told you that Tactics cannot host old Dashboard instruments because of the limited space provided by 32 "Dashboard sentences"? This instrument has taken the toll. Take a look at https://github.com/tom-r/tactics_pi/blob/e97aede98dc203e6ec1f1ce17c6ff19947fe1b09/src/tactics_pi.cpp#L252 and you will see that it is impossible to select Rudder instruments in Tactics v1.0.11!

Call to it commented out as well: https://github.com/tom-r/tactics_pi/blob/e97aede98dc203e6ec1f1ce17c6ff19947fe1b09/src/tactics_pi.cpp#L2435

It is compiled on every build, though: https://github.com/tom-r/tactics_pi/blob/master/CMakeLists.txt

rgleason commented 4 years ago

I am very sorry. In haste I selected the recent but old version of DashT. You are absolutely right. The instrument does not appear in Tactics. I am closing it. Very sorry.

Its a wonder why Thomas doesn't move on to the 64 instruments, I guess he wants to keep it identical to the old Dashboard 32 instruments...

ghost commented 4 years ago

No problem, to change from int (32) to long long (64) everywhere in the extremely large code base is a job which is insanely... well,. insane! I can't recommend it to anybody. It would be better to complete revamp both the internal data format (guess, SK delta ... 😀 ) and how each individual instrument subscribe to that data. I read the thread you indicated and the answer is right there, in front of our eyes, in one of the comments of teppokurki: subscribe to a delta sentence by creating a handler for it! Beautiful! This way, like the instruments would be floating in a complete freedom,each in their own universe. Plug-in would just carry them, start them. I think this could be done without throwing the entire existing code in the bin, just create new instruments and replace the old ones with the new ones. Evolution, not revolution! Like with the plug-ins, you keep the old ones and develop new ones in addition.