OHM Axial Velocity HUD v1.0 RC1

[agentgonzo]

I see.

I used the assembly approach to ensure one single patch-access during runtime.

Dropping out to this won't protect against race conditions as you can still get preempted in your assembler block - you've just made it more efficient so reduced the number of instructions to perform the patch, so reduced the chance of getting preempted in the code block.

A much more reliable way would be to use Mutexes to ensure that only one thread has access to your sections of code. Unless you've been using the TestAndSet instruction in your assembler to essentially run your own mutexes (but from what I can remeber of the code of OMP, it doesn't).

Btw, my hooking code is a bit simpler and does not involve assembly.

Neither did mine. You can see the LaunchMFD source here (in case you hadn't already been there)

 

[Face]

Dropping out to this won't protect against race conditions as you can still get preempted in your assembler block - you've just made it more efficient so reduced the number of instructions to perform the patch, so reduced the chance of getting preempted in the code block.

A much more reliable way would be to use Mutexes to ensure that only one thread has access to your sections of code. Unless you've been using the TestAndSet instruction in your assembler to essentially run your own mutexes (but from what I can remeber of the code of OMP, it doesn't).

Just one thing: the assembler code within the hooked functions is not performing a patch of the VMT the way Wraith's code is doing it. It is performing a call to the original function, replacing the vessel-object pointer while doing so. The VMT - once patched - stays the same all the time. Therefore, concurrent threads modifying seperate objects of the same class can't collide (regarding VMT access of course, not regarding e.g. static members). With the repatching method, you would have to ensure a single point of access to every hooked class-method, somewhat breaking the object-oriented approach.

Anyway, you're right, it doesn't protect against race-conditions per se. Of course I used locking in this case. Repatching of the VMT was causing problems with the locking, making it more complex. Hence I eliminated repatching...

That said, I know that the hooking-class is far from being perfect, it is just a different approach to the classic repatching.

regards,
Face

 

[Wraith]

The VMT - once patched - stays the same all the time. Therefore, concurrent threads modifying seperate objects of the same class can't collide (regarding VMT access of course, not regarding e.g. static members). With the repatching method, you would have to ensure a single point of access to every hooked class-method, somewhat breaking the object-oriented approach.

Correct me if I'm wrong, but I always thought that Orbiter is neither mutithreaded nor multithreading-aware, and forcefully making more than one thread inside Orbiter (any part) can cause race contitions inside. For instance, one starts calling methods for two separate vessels simultaneously, but what if one vessel calls into another for some reason, eg. knowing its position/velocity/nav transmitters/whatever, while the other changes those values? That's a race condition here. I don't believe there's either any kind of internal locking in all those custom vessels, or any kind of locking inside Orbiter's core. What's the use of multithreading here?

But anyway, are we in agreement that starting the VMT Hijackers League (VHL) is the way to go? If so, shall we discuss the design of the "Standard" VHL API that certain developers will have to follow?

 

[doggie015]

I may have found a little bug. In DGIV, safe mode, power and hud turned off. HUD refuses to turn on. If the mission is saved with HUD on, every thing works fine. Can someone confirm this.

Umm... it's in safe mode, The DGIV is MADE that way to add to realism; what is the point of having a HUD if there is no-one onboard to use it!

 

[Wraith]

Umm... it's in safe mode, The DGIV is MADE that way to add to realism; what is the point of having a HUD if there is no-one onboard to use it!

No, actually that was a bug in vel_hud, and was fixed in RC2.

 

[Face]

Correct me if I'm wrong, but I always thought that Orbiter is neither mutithreaded nor multithreading-aware, and forcefully making more than one thread inside Orbiter (any part) can cause race contitions inside. For instance, one starts calling methods for two separate vessels simultaneously, but what if one vessel calls into another for some reason, eg. knowing its position/velocity/nav transmitters/whatever, while the other changes those values? That's a race condition here. I don't believe there's either any kind of internal locking in all those custom vessels, or any kind of locking inside Orbiter's core. What's the use of multithreading here?

You're right. The reason for me doing it multithreading-safe was OMP being multithreaded. It uses one thread for receiving stream-information, one thread for sending information, one for synchronizing client and so on...
Of course those threads and Orbiter's core-execution are interlocked (by using windows semaphores). Of course the critical sections within the threads are locked, too. But operations on different vessel-objects (of the same class - say, 2 DGs) are not interlocked, e.g. opening the nose-cone by clicking on the appropriate area in the virtual cockpit. This can happen simultaneously on the local side and the remote side, with the local side actuator inside the Orbiter-engine and the remote actuator in the receiver-thread.

Of course Orbiter itself is not multithreading-safe, so you have to take care of it.

My point is, that the repatching of VMT in order to call the original-function is a class-wide operation, whereas calling the original-function from a table with altered object-pointer is a object-wide operation. If you are performing class-wide operations you have to serialize multithreaded access to the function. If you are performing only object-wide operations you have to serialize multithreaded access to the object.

Unfortunately I only thought of assembler for calling the original class-function just with altered object-pointer. If there is a better way to do that, I'd be happy to hear about it. Repatching the VMT is something different...

But anyway, are we in agreement that starting the VMT Hijackers League (VHL) is the way to go? If so, shall we discuss the design of the "Standard" VHL API that certain developers will have to follow?

Absolutely. But as stated before, I'm sure the best solution for future developments would be core-support in the spirit of the old oapi-callbacks. I'll try to talk with Martin about it, it surely is an important API-issue with all those "HudModeXForAll" addons coming up.

regards,
Face

 

[Wraith]

My point is, that the repatching of VMT in order to call the original-function is a class-wide operation, whereas calling the original-function from a table with altered object-pointer is a object-wide operation.

Unfortunately I only thought of assembler for calling the original class-function just with altered object-pointer. If there is a better way to do that, I'd be happy to hear about it.

Oh, now I do understand why the magic.

Yes there could be yet another way: we could try to hijack an entire VMT for each object -- because each instance has a separate pointer to the VMT, so copying entire table somewhere and patching the pointer could do. AFAIK, each VMT ends with zero, so we should know how much to copy.

But as stated before, I'm sure the best solution for future developments would be core-support in the spirit of the old oapi-callbacks. I'll try to talk with Martin about it, it surely is an important API-issue with all those "HudModeXForAll" addons coming up.

Of course, but that's not going to happen until the "next Tuesday", as I get it. Our hacking shall be a relatively short-term solution, while adding new features to Orbiter itself is the long-term one.

 

[Face]

Oh, now I do understand why the magic.

Yes there could be yet another way: we could try to hijack an entire VMT for each object -- because each instance has a separate pointer to the VMT, so copying entire table somewhere and patching the pointer could do. AFAIK, each VMT ends with zero, so we should know how much to copy.

Indeed. The only drawback I see here is the need to do this for every instance you like to hook. OTOH, my class is doing this implicitely, too, just not inside code memory (i.e. by patching the objects pointer to the VMT).

This could be a good solution to get rid of the assembler part.

Another thing: I already thought about patching Orbiter's code itself, installing a hook in the HUD-drawing function. This way, the custom HUD can work with standard VESSEL objects, too, not just with VESSEL2. Now this would really call for a singleton handler API...

Of course, but that's not going to happen until the "next Tuesday", as I get it. Our hacking shall be a relatively short-term solution, while adding new features to Orbiter itself is the long-term one.

True. So your idea is a general, centralized system for hooking all VESSEL2 callbacks? Or - to shorten the release time - just a stable toolkit to hook DrawHUD?

Also, since agentgonzo did most (if not all) of the HUD code in LaunchMFD (IIRC) and tried several other approaches to the HUD-draw problem (including API-hooking into DX itself), I'd like to hear what solution he would come up with.

regards,
Face

 

[Wraith]

True. So your idea is a general, centralized system for hooking all VESSEL2 callbacks? Or - to shorten the release time - just a stable toolkit to hook DrawHUD?

Right now I don't see any significant difference between the two. And I don't think it'll take much time implementing that.

Here's what I propose.
Please, see if it also suits your needs, eg. try to write a simple use case and see if it will do. If you have anything to add -- just go ahead and edit the code.

Also, I'd like to point out, that IMO the interface should be as simple as possible, so that making a new version won't break any would-be existing addons.

So, if there are potential VMT hijackers reading this thread -- your comments are welcome.

 

[tblaxland]

So, if there are potential VMT hijackers reading this thread -- your comments are welcome.

I've been quietly watching this thread but I'll jump in now. I like your idea of a VHL API and would consider using it in the future. Looking at what you posted above I can see how to use it easy enough, so that's all good. I must admit I'm a little confused as to the specifics of the implementation that you've been discussing above so I think I'll sleep on that . Good work guys.

 

[agentgonzo]

Also, since agentgonzo did most (if not all) of the HUD code in LaunchMFD (IIRC) and tried several other approaches to the HUD-draw problem (including API-hooking into DX itself), I'd like to hear what solution he would come up with.

You recall correctly. I think that Enjo added some drawing to the HUD, but all of the hooking was done by me. The three methods that I tried (that I can remember) were:
1) Setting a timer to draw to the HWND directly
2) Hooking DX itself
3) Hooking the VMT of VESSEL2 (what I settled on).

1 - Setting a timer and drawing to the HWND of the main window didn't work too well. This is what AutoFCS uses to draw on the HUD. As it's a timer, it isn't guaranteed to be called after the drawing is done and before the surfaces are flipped. This resulted in a nasty flicker on the screen for the times when the timer didn't fire and so no HUD was drawn. It's OK for small things (like the AFCS flight flight director) as the flicker isn't too noticable, but for larger things it began to give me a headache. Also, it seemed to draw lines 2 pixels wide rather than 1 - probably because it was called twice on the same frame with slightly updated coords. I didn't look into this too much.

2 - Hooking DirectX itself. I can't rememeber too much about this one. I think that I was trying to hook the EndScene() function, but I was having trouble (either compile or runtime) with one of the DirectX classes. I can't remeber exactly what the problem was, but it was at the last stage (you need to overload about 3 DX classes to get a handle on the D3D object that has the EndScene function you can hook. I gave up on this in the end as I couldn't figure out what was going wrong. If I had got it to work this way, then it would have worked with vel_hud (as they would have hooked separate functions) but then if someone else implemented the DX hooking in the same fashion in another MFD, then we'd be back to the same situation as I think it hooked the EndScene function in exactly the same way.

Edit: Here is the summary of the DX hooking that I attempted, and where it failed.


-----Posted Added-----

Here's what I propose.

If you're going to do this, you won't just be able to cut-and-paste this functionality into each MFD. Even if you implemented it as a singleton class and just cut-and-pasted the code into each MFD, they'd get compiled into each MFD DLL as a separate bit of code and you'd end up with 2 or 3 identical implementations of singleton classes that would each try and hook the HUD callback assuming they were the only ones.

You'd need a separate DLL (HUDHook.DLL or something like that) that each of the prospective HUD Hookers would link to and call the one hooking function from that to make sure that there is only one hook object instantiated and only one hooking performed. You'd then include the HUDHook.DLL with every release that used it (though that's not a problem really).

If you're going down the route of one separate library hooking DLL then you'll want to make the exported functions simple and not change. You'll also want to future-proof it, so while you're at it, you may as well add the functionality to hook *any* of the virtual methods of VESSEL2.

 

[Face]

If you're going to do this, you won't just be able to cut-and-paste this functionality into each MFD. Even if you implemented it as a singleton class and just cut-and-pasted the code into each MFD, they'd get compiled into each MFD DLL as a separate bit of code and you'd end up with 2 or 3 identical implementations of singleton classes that would each try and hook the HUD callback assuming they were the only ones.

You'd need a separate DLL (HUDHook.DLL or something like that) that each of the prospective HUD Hookers would link to and call the one hooking function from that to make sure that there is only one hook object instantiated and only one hooking performed. You'd then include the HUDHook.DLL with every release that used it (though that's not a problem really).

If you're going down the route of one separate library hooking DLL then you'll want to make the exported functions simple and not change. You'll also want to future-proof it, so while you're at it, you may as well add the functionality to hook *any* of the virtual methods of VESSEL2.

Agreed. I see Wraith's proposal as interface, though, not as actual implementation. I too think, that an implementation similar to DanSteph's OrbiterSoundSDK or UMMU-lib should be used. I.e.: late binding of library functions and strict interface versioning. Therefore your comment on future-proof is pretty spot-on, IMHO.

Wraith: I edited the proposal by adding an additional function (vhlDrawHUD) to the concept. Of course, the actual implementation needs to take into accout the "wrong" function table at the time the hooked function is called. I think an API should care about the specific actions taken to call the original function and make it transparent to the API-user. Within the proposed vhlxxx-function, the user doesn't have to mind subsequent original-function calls.
Additionally I removed the HOOK type. I think the interface is cleaner that way, as you already have the custom hook_class as key. I don't see the need for an addition handler.
Just a proposal, of course...

I already thought of something in the line of "remove_all_hooks", or "cleanup_system", too. But I think the implementation should take care of this instead of the actual user, and - most of all - the user should not be able to remove hooks of other addons.

regards,
Face

 

[agentgonzo]

I already thought of something in the line of "remove_all_hooks", or "cleanup_system", too. But I think the implementation should take care of this instead of the actual user, and - most of all - the user should not be able to remove hooks of other addons.

The remove_all_hooks should definitely be part of the implementation, rather than the interface for the reason you state. You may want to add a remove_all_hooks function for a specific MFD though, so the MFD has a one-line cleanup state, rather than having to call it for each method it has hooked.

 

[Wraith]

If you're going to do this, you won't just be able to cut-and-paste this functionality into each MFD....
You'd need a separate DLL (HUDHook.DLL or something like that) that each of the prospective HUD Hookers would link to and call the one hooking function from that to make sure that there is only one hook object instantiated and only one hooking performed. You'd then include the HUDHook.DLL with every release that used it (though that's not a problem really).

Well, that's exactly what I meant when I spoke of a "Standard Hijacking API". Indeed, a DLL, much like OrbiterSound, that does the dirty job and provides a clean interface to the interested developers.

If you're going down the route of one separate library hooking DLL then you'll want to make the exported functions simple and not change. You'll also want to future-proof it, so while you're at it, you may as well add the functionality to hook *any* of the virtual methods of VESSEL2.

I think I didn't explain that properly -- the proposal is already to hook the entire interface, not a single method. Internally the DLL shall hijack every single method of VESSEL2 interface for the given instance. The developer should implement the method he wants to hook, which will override the default from VHL::hook_class and call the VHL::add_hook(), that adds the developers hook to the internal list of hooks for that particular vessel instance.
All hooks shall be managed by the implementation, so a particular developer shall not be needing to worry about eg. calling other hooks.

Face: I think I caused a great deal of confusion with that hook_class.
The idea is that this hook_class is an interface between the developer and the VHL API DLL. It's more like an event subscription. That is, by overriding these functions in a hook_class-derived class the developer can respond to various callbacks. The VHL API DLL shall internall maintain the list of pointers to these interfaces, but those are not interfaces to the original vessel instance.

There shall be only one "Master Hook" for each instance, hidden inside the VHL API DLL. When clbkXXX() is intercepted, that "Master Hook" shall notify its subscribers by calling their appropriate handlers through supplied ( through add_hook() ) interface pointers.

This way a particular developer won't have to worry about chaining the call to other hooks -- the implementation shall manage that. I don't think that emptying the internal subscriber list should be allowed, as this will simply screw up the other addons.

That magic cookie is just a way to unsubscribe from a particular vessel instance. No big deal though.

 

[Face]

Face: I think I caused a great deal of confusion with that hook_class.
The idea is that this hook_class is an interface between the developer and the VHL API DLL. It's more like an event subscription. That is, by overriding these functions in a hook_class-derived class the developer can respond to various callbacks. The VHL API DLL shall internall maintain the list of pointers to these interfaces, but those are not interfaces to the original vessel instance.

There shall be only one "Master Hook" for each instance, hidden inside the VHL API DLL. When clbkXXX() is intercepted, that "Master Hook" shall notify its subscribers by calling their appropriate handlers through supplied ( through add_hook() ) interface pointers.

This way a particular developer won't have to worry about chaining the call to other hooks -- the implementation shall manage that. I don't think that emptying the internal subscriber list should be allowed, as this will simply screw up the other addons.

That magic cookie is just a way to unsubscribe from a particular vessel instance. No big deal though.

Ah, now I understand. So your hook_class already is the custom part of the hook. No need for the vhlxxx-callbacks, then...

agentgonzo: If we want to implement the functionality to remove all hooks from a vessel (or all vessels) only defined by a specific MFD/Plugin, we need a way to identify the MFD/Plugin. That would lead to a kind of registration again similar to DanSteph's work, i.e. the MFD/Plugin needs a handle to access the hooking system, adding one more parameter to the function calls.

regards,
Face

 

[Wraith]

Ah, now I understand. So your hook_class already is the custom part of the hook. No need for the vhlxxx-callbacks, then...

Yep, that the deal.

 

[Wraith]

Ok, after playing for a while with entire VMT hijacks, all I can tell that it works ok -- changing a per-vessel-instance VMT pointer definitely does the trick, so I guess class-wise locking can be easily avoided.

New interface version http://www.everfall.com/paste/id.php?gzhiqlsmna1x.

Some notes:

I dropped the namespace, so that the DLL can be loaded dynamically, and function addresses can be easily obtained via GetProcAddress(), eg. by the installer to check its version.

The user interface isn't VESSEL2-derived anymore, as that's of no use. Event sink is-not-a vessel after all.

I decided to make the each method hook twofold, i.e. one function is a pre-call, the other is a post-call, because one might need either or even both.

VMT_HSUB (previously VMT::HOOK) -- although not crucial, its use will simplify searching for the right entry in the hooks list to an instant O(1) (think of an std::list<>::iterator).

Also, I'm thinking whether allowing a user-define event sink to change the original returned value from a callback (in on_ret_xxx() ) is a good idea.

---

On another topic, I wonder if OBJHANDLE values are unique at least during one session, or if they are reused at some point. Could someone shed some light on this?

 

[tblaxland]

New interface version http://www.everfall.com/paste/id.php?gzhiqlsmna1x.

Looks pretty good to me.

Also, I'm thinking whether allowing a user-define event sink to change the original returned value from a callback (in on_ret_xxx() ) is a good idea.

I can't think of a use for it right now but it would be a good feature to have for the sake of completeness and future-proofing - assuming of course it is not too hard to acheive

On another topic, I wonder if OBJHANDLE values are unique at least during one session, or if they are reused at some point. Could someone shed some light on this?

I don't know on this one. There is nothing I can find to indicate that they have guaranteed uniqueness, so it would probably be a bad idea to rely on it.

 

[Wraith]

Looks pretty good to me.
I can't think of a use for it right now but it would be a good feature to have for the sake of completeness and future-proofing - assuming of course it is not too hard to acheive

Yeah, but what bothers me is that if two hijackers decide to alter the returned value, the outcome would depend loosely on who was the last to add an event sink to the vessel, which in turn may depend on the order of addons in orbiter.cfg. And that's pretty bad, don't you think?

 

[tblaxland]

Yeah, but what bothers me is that if two hijackers decide to alter the returned value, the outcome would depend loosely on who was the last to add an event sink to the vessel, which in turn may depend on the order of addons in orbiter.cfg. And that's pretty bad, don't you think?

That's true. Thinking about it some more, it is also pretty bad to change the return value that the vessel coder obviously wanted to be returned. In other words, if you change the return value, you may break vessel functionality for some vessel instances (the hooked ones) and not others. So I change my vote to not implementing this option.

A VMT Hijacker would need to be careful with hooking clbkConsumeDirectKey and clbkConsumeBufferedKey as these could potentially break vessel functionality. Is there any reason why the char *keystate argument couldn't be changed to const?

    virtual void on_clbkConsumeBufferedKey ( VESSEL2* vess, DWORD key, bool down, [B][COLOR=Red]const [/COLOR][/B]char *keystate ) {}
    virtual void on_ret_clbkConsumeBufferedKey ( VESSEL2* vess, int, DWORD key, bool down, [B][COLOR=Red]const [/COLOR][/B]char *keystate ) {}

And perhaps also generally we could change the VESSEL2* to const also?