-
Notifications
You must be signed in to change notification settings - Fork 31
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
Custom steering vector transform #38
Comments
So I prototyped this out a bit in my own repository. I'm going to do some testing on it with both the null transform and my own transform function over the next few days. I'm not very familiar with GitHub, so I'm not really sure how to best show you my changes though. Never really done any sort of collaborative coding before. |
Please excuse my poor coding skills, if i understand correctly, i have to add these lines below the current lines. Is that correct ? I would be happy to help you but only for tests. sorry :) |
No, it's going to take more than just that. I'm going to do some testing and then open a pull request that should cover the needed changes, at least as I have them prototyped. I'm sure Noiredd will have some changes/suggestions once I do before it's ready. |
@lapiante For now the functionality isn't even implemented, so using these few lines won't do anything. @Jakathan Before we rush to coding and PRs, let's figure out how exactly we want to handle this. Initially I was thinking that I'd implement the PEGAS-side feature, but that might take me a few days. If you want to do all that - fine with me. But the more important decision is regarding the actual delegate: should PEGAS come equipped only with the most basic version (i.e. nullTransform), leaving it for the user to implement their own transforms, or is it possible to provide a built-in transform? What's your opinion on this? |
I guess that's a question for you to answer, as it's your project 😄 I'm more than happy to let you have my steering transform function to include with PEGAS. I think that would be the most common one people might have use of, as well as the one that should be most plug-and-play. It would also be a good example of what one could do with the functionality. As far as switching transforms mid flight, shouldn't it be as easy as using a delegate event in your sequence to change the steering transform variable to whatever new transform you want?. You could also create a specific event for that, but I don't think that's necessary. |
It's my project, but since there are others using it besides me, I feel that some changes - especially those that come suggested by the community - should be discussed with this community :)
This gives me an idea. What if the bare functionality (i.e. delegate handling) but no delegates themselves were implemented in PEGAS directly, but an example usage - i.e. your function - be included in the documentation? This way the "framework" code would not bloat, but users with these particular needs could still have what they want.
It feels a bit like a monkey-patch to me :P But I suppose that should work. I'd expect a very small fraction of users to ever need that to begin with, so creating a whole new facility (like a specific event just to handle this) feels like a waste of time (and a code bloat). Alright I'm going to implement the base feature the next time I've got any free time, then we'll talk about including your example, @Jakathan - I think it would be good if you authored a commit including this, to take your deserved credit. |
I think this is a great solution! This gives a practical example of how you could use the functionality without complicating it too much for someone who just needs the base code. It would also give an opportunity to reiterate the need to avoid dead delegates, like you do in the Sequence tutorial section. Let me know when you have the delegate handling implemented and I can start writing some example documentation. |
@Jakathan I've just came up with a concrete idea for addons for PEGAS. Please check out #33 and tell me what you think of it in the context of this transform. Maybe we could get away without building it into the main code itself, but instead it could live as an addon? Here's what I'm thinking:
Following what I outlined in #33, this would make your function be called after every iteration of UPFG (about this line, for example). The only potential problem I see with this approach is that there would be some non-zero delay between UPFG calculating the base steering vector (bear in mind, steering is locked to this variable) and recalculation by the post-loop hook. I think this time would be negligible (i.e. not enough for kOS to start rotating the vehicle to the pre-correction vector before the correction kiks in), but maybe I'm wrong. If it worked however, it could be a nice use for the addon mechanism without burdening the existing codebase and, especially, documentation :) Let me know what you think! |
I quite like this approach. I looked over your comments in #33 and it seems to be a pretty elegant solution, especially as a general way to add in custom modules. I haven't examined script timing too closely yet, so I'm not sure how quickly the loop runs, but I agree that it will probably be negligible. Upping the IPU value would also help prevent that somewhat, as you're nowhere near maxing that out yet. When you get it implemented, I will test it out a little bit. I'm going to try and launch a rocket with the probe core pointing backwards, so any delay between the initial and corrected steering should be more obvious. Also just to prove that I can... |
Addons are here! Now we only need to discuss one thing: should this functionality be highly configurable (i.e. using the above proposed system involving pulling a delegate from vehicle or mission configs), or would it suffice to just have your In either case, I'd really like to leave this up to you - whatever you implement that works for you, I'd invite you to share it as an addon in the addon repo :) |
I'm not sure exactly which module you're referring to in the 2nd approach. As far as implementing this as an addon, I've rewritten it so that it can be used as such and have done a bit of testing. I need to write the documentation for it, but once I get a chance to do that, I can add it to the addon repository. Although Patrykz94 already beat me to the first addon... |
In the first approach, this addon would retrieve the vector transforming delegate from The second approach comes with the transformation function already built in. So the vehicle description lexicons do not have to include it, it's easier on the user. It could still be configurable (so that one doesn't have to install/uninstall the addon between different flights), e.g. enable the transformation if And don't worry about not being the first ;) Patryk's addon is a culmination of the idea he proposed as far back as 2017, then implemented as a built-in feature proposal in 2020 - I guess it's fair that his idea got there first :) |
Hello, |
@lapiante No, not yet. @Jakathan If you're still interested in making this happen, I have a perfect and simple idea to solve this as an addon. It would consist of 2 hooks. One remains the same as a few comments up, something bound to
This would sort out all the problems with IPU and whatever. Furthermore, the addon itself could read the transform from vehicle info: Thoughts? |
It has been pointed out that simple steering locked to a vector calculated by UPFG breaks for some vehicles. The solution is to apply a vehicle-specific transformation of the steering vector that accounts for the vehicle center of mass, and then locking the steering to that.
This should be easy to implement using an additional layer of abstraction between UPFG output and kOS cooked steering. An additional function would input the raw UPFG steering vector and output a real vector that steering would be then locked to. This function would default to an identity transform:
But it could be overridden by the user in the form of a delegate, for example passed through the
CONTROLS
lex.Seems like a simple fix, but help with testing would be appreciated.
The text was updated successfully, but these errors were encountered: