search

Boris-Barboris / AtmosphereAutopilot

Plugin for Kerbal Space Program
GNU General Public License v3.0
48 stars 16 forks source link

Fix for NeoGUI speed slider #17

Closed radistmorse closed 7 years ago

radistmorse commented 7 years ago

"Single structure" he said. "Easy to extend" he said. "What's not to like" he said. Does this look incapsulated to you? NO, it's not. Sorry, but this is a bad design. I just got my PhD, you can trust me :)

Also, look here: https://github.com/Boris-Barboris/AtmosphereAutopilot/blob/master/AtmosphereAutopilot/Modules/ProgradeThrustController.cs#L532 You are creating a new structure every fucking frame! For no reasons whatsoever. And you still think that a meaningless structure is a best choice?

Boris-Barboris commented 7 years ago

In my opinion, first thing you should have learned in university is to keep your degree for HR department in any argument you don't wish to come out of as arrogant prick.

Secondly, I seems like you don't need to touch setpoint_field, wich is incapsulated allright. The only thing there is to change is https://github.com/Boris-Barboris/AtmosphereAutopilot/blob/master/AtmosphereAutopilot/Modules/ProgradeThrustController.cs#L390 : this boolean flag, that was intoduced not by me. Don't touch neither chosen_speed_mode, nor setpoint_field, and set or unset spd_control_enabled when handling a click on that toggle near speed control in you GUI. You are the one breaking incapsulation by touching other GUI system related fields.

Upd: this is how upper level controllers call thrust controller: https://github.com/Boris-Barboris/AtmosphereAutopilot/blob/master/AtmosphereAutopilot/Modules/StandardFlyByWire.cs#L204 Notice that those fields adressed are not GUI-related.

Considering your second paragraph, I shall advise you once again to revise both conceptual and technical differences between classes and structs in C#, you are clearly thinking something awful is happening under that "new" directive, wich is not the case. It would be awful in C++, but not here.

Upd: you may ask, why not simply change value, instead of calling constructor again? That is simply to respect Microsoft's value tytpe paradigm, nothing more. Also, such choice is less prone to change should the setpoint become class property instead of plain field.

Yes, I think it is a good choice, it would be bold to claim such choice "the best" though. If you have a vision on the subject and want to encorporate it into AA, feel free to implement the pattern you like, I'm sure after a couple of iterations we'll come to successfull merge.

Boris-Barboris commented 7 years ago

On the second thought... Are you sure it doesn't work in the first place? I'm currently controlling it allright using the source from master branch. Could it be the specific UI dll from release zip was not recompiled?

Upd: no, still works OK for me with dlls from release zip.

radistmorse commented 7 years ago

Kind of a mixed message here :) Anyway, it doesn't work when you enable both GUIs. The reset happens in SpeedCtrlGUIBlock () Moreover, even if you change the speed with NeoGUI, and then at some point enable the old one, the moment it pops on screen the speed limit resets back.

Boris-Barboris commented 7 years ago

I think it's kinda too much to make both GUI's work well with each other. Surely, it's possible, but why would you want to?