Skip to content
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

elevmgr: added clbkFilterElevation #24

Merged
merged 1 commit into from Aug 4, 2021

Conversation

Face-1
Copy link
Contributor

@Face-1 Face-1 commented Aug 1, 2021

Clients that manipulate elevation file data in memory (e.g. for
flattening features) for visuals can overload this method in order to
manipulate the terrain collision data in the same way. As soon as the
internal collision tile is loaded in the core, the callback is invoked.

Clients that manipulate elevation file data in memory (e.g. for
flattening features) for visuals can overload this method in order to
manipulate the terrain collision data in the same way. As soon as the
internal collision tile is loaded in the core, the callback is invoked.
@krainboltgreene
Copy link

If I may offer an alternative name for this function: onFilterElevation. on makes immediate sense over clbk, which is a unusual shorthand for callback, and is very common (see similar browser hooks).

@schnepe2
Copy link
Contributor

schnepe2 commented Aug 2, 2021

I would stay with the clbkXxx naming theme as it is consistent with all the other method names.
If we would like to change to the onXxxx theme, all other methods should be renamed as well (I don't think this is a benefit)

@krainboltgreene
Copy link

Ah, if there is already a pattern then yes, best to stick to the pattern no matter how bad it could be for end users.

@Face-1
Copy link
Contributor Author

Face-1 commented Aug 3, 2021

Ah, if there is already a pattern then yes, best to stick to the pattern no matter how bad it could be for end users.

I don't know how familiar you are with Orbiter and its ecosystem, so I try to explain it a bit further.

The proposed change is not affecting end users directly. Orbiter is no browser add-in or something like that, it is a core physics implementation that offers an API to add-on developers. The proposal at hand specifically targets the graphics API, which in turn is used by developers of so-called OVP (Orbiter Visualization Project) clients. These clients are the graphics end of the simulator.

One particularly important client (the de-facto standard these days) is D3D9Client, and in this project, there is a feature that offers end users the ability to flatten terrain by simple text-based configuration files. However, there is the problem of the core loading and interpreting terrain data in parallel to the visualization clients, resulting in dissonance between visual and collision experience when the feature is used. In the past - due to the closed source - the only possibility to overcome this problem was to hook the core process in order to implant the above detailed callback by means of a hack. It worked, but was always hard to maintain (new versions needed new reverse-engineering and careful trampoline crafting). The proposal here gets rid of this hack in a minimal invasive way.

Because the API uses the name scheme since like 20 years or so, it would actually harm end users if OVP developers get confused with an out-of-band naming for this simple callback. If the addition of the callback is hold back until a name-change of the whole API is done, it will harm end users because OVP developers need to stick to the hack, or even deactivate the feature that only came in due to end user pressure in first place.

@krainboltgreene
Copy link

That's why I wrote "best to stick to the pattern".

@mschweiger mschweiger changed the base branch from main to face_elevmgr August 4, 2021 00:54
@mschweiger mschweiger merged commit b2ca18e into orbitersim:face_elevmgr Aug 4, 2021
@mschweiger mschweiger mentioned this pull request Aug 4, 2021
@Face-1 Face-1 deleted the feature/FilterElevation branch August 4, 2021 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants