Skip to content

Conversation

@Beyley
Copy link
Contributor

@Beyley Beyley commented Mar 22, 2023

Summary of the PR

Closes #1333
Allows passing multiple library paths to CreateDefaultContext, and add multiple library path support into SearchPathContainer

Related issues, Discord discussions, or proposals

Links go here.

Further Comments

This also brings into consideration the possibility cleaning up of places such as OpenAL, so that it tries the native OpenAL, then later tries OpenAL-soft, instead of the "choose one or the other" that we have now,
@Perksey should the AL.GetApi(bool) be deprecated with ObsoleteAttribute and the new non-deprecated behaviour be Native Platform -> Soft?

@Beyley Beyley force-pushed the feature/multiple-library-paths branch 2 times, most recently from 0abd791 to 1671ba4 Compare March 22, 2023 06:23
@Khitiara
Copy link
Contributor

re: openal id like the ability to choose to prioritize soft but its not a huge deal

@Beyley
Copy link
Contributor Author

Beyley commented Mar 23, 2023

re: openal id like the ability to choose to prioritize soft but its not a huge deal

perhaps it should be changed to "prioritize" instead of "only soft" or "only native", true means prefer soft, then try native, false means prefer native, then try soft

@Perksey
Copy link
Member

Perksey commented Mar 23, 2023

I agree with this, prioritise makes sense.

@Beyley Beyley force-pushed the feature/multiple-library-paths branch from 4cadbd0 to 2e35013 Compare March 24, 2023 19:18
@Beyley Beyley marked this pull request as ready for review March 24, 2023 19:23
@Beyley Beyley requested review from a team, HurricanKai and ThomasMiz as code owners March 24, 2023 19:23
Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think overrides should be activated whenever the list contains something.

I don't have all the info on this, but in my mind in there the candidate to search for should've already been decided and simply loaded, with some logic higher up iterating through the array and finding the first functional candidate.

I think it's reasonable to account for a situation where one possible candidate is __Internal which needs the override, but another candidate can simply use the normal logic and should not also trigger the override.

I'm also not sure what happens it a candidate not ment to hit an override ends up in the override branch. They do all sorts of casting insanity to be efficient and trick the JIT into accepting invariants that are not immediately obvious otherwise. Breaking those might cause all sorts of bugs that I can't foresee anymore.

@Beyley
Copy link
Contributor Author

Beyley commented Mar 24, 2023

I don't think overrides should be activated whenever the list contains something.

I don't have all the info on this, but in my mind in there the candidate to search for should've already been decided and simply loaded, with some logic higher up iterating through the array and finding the first functional candidate.

I think it's reasonable to account for a situation where one possible candidate is __Internal which needs the override, but another candidate can simply use the normal logic and should not also trigger the override.

I'm also not sure what happens it a candidate not ment to hit an override ends up in the override branch. They do all sorts of casting insanity to be efficient and trick the JIT into accepting invariants that are not immediately obvious otherwise. Breaking those might cause all sorts of bugs that I can't foresee anymore.

ah yeah i completely blanked on the override problem, thats an interesting one to solve hmm

@Beyley
Copy link
Contributor Author

Beyley commented Mar 25, 2023

@Perksey do we just not allow multiple libnames on platforms with P/Invoke overrides? i cant think of a way this can really be done cleanly, unless we want to try all libnames as dynamically loaded using our dynamic loader first, and if that fails, then use the P/Invoke override, unless it is possible that the P/Invoke override may be detected as being able to be dynamically loaded, but shouldnt be for x or y reason

@Perksey
Copy link
Member

Perksey commented Mar 25, 2023

I recommend:

  • Adding a static function in DefaultNativeContext called TryCreate. You will probably need to do the same in UnmanagedLibrary and probably something in LibraryLoader as well. The goal here is to make a code path that does not throw when failing to load.
  • Iterating through each item in the names array
  • Attempting to load each item using the same logic as in CreateDefaultContext today (but using the new non-throwing method), and moving onto the next candidate if this fails.

This way the priority is still respected.

@HurricanKai
Copy link
Member

I agree with perksey. Attempting different candidates and deciding which ones to use should be done at a higher level then overrides.

@Beyley Beyley force-pushed the feature/multiple-library-paths branch from 3f75474 to ad1099e Compare March 25, 2023 22:05
Beyley added 3 commits March 25, 2023 15:05
This means that now if loading OpenAL soft fails, ittl try to fall back to native, and if loading OpenAL native fails, ittl fall back to OpenAL soft
@Beyley Beyley force-pushed the feature/multiple-library-paths branch from ad1099e to c9bd443 Compare March 25, 2023 22:06
@Beyley
Copy link
Contributor Author

Beyley commented Mar 26, 2023

I recommend:

* Adding a static function in DefaultNativeContext called TryCreate. You will probably need to do the same in UnmanagedLibrary and probably something in LibraryLoader as well. The goal here is to make a code path that does not throw when failing to load.

* Iterating through each item in the names array

* Attempting to load each item using the same logic as in CreateDefaultContext today (but using the new non-throwing method), and moving onto the next candidate if this fails.

This way the priority is still respected.

This has now been implemented, with the codegen of

        [System.ObsoleteAttribute("This function is obsolete! Please use the string[] overload!")]
        public static INativeContext CreateDefaultContext(string n)
        {
            if (n == "libSDL2.so")
                return new OVERRIDE_1();
            else if (n == "__Internal")
                return new OVERRIDE_0();
            else
                return new Silk.NET.Core.Contexts.DefaultNativeContext(n);
        }

        public static INativeContext CreateDefaultContext(string[] n)
        {
            foreach (string name in n)
            {
                if (name == "libSDL2.so")
                    return new OVERRIDE_1();
                else if (name == "__Internal")
                    return new OVERRIDE_0();
                else if (Silk.NET.Core.Contexts.DefaultNativeContext.TryCreate(name, out DefaultNativeContext context))
                    return context;
            }

            throw new System.IO.FileNotFoundException("Could not load from any of the possible library names! Please make sure that the library is installed and in the right place!");
        }

@Perksey
Copy link
Member

Perksey commented Mar 28, 2023

Perfect, thank you!

@Perksey Perksey merged commit 08613f6 into main Mar 28, 2023
@Perksey Perksey deleted the feature/multiple-library-paths branch March 28, 2023 19:05
Beyley added a commit that referenced this pull request Mar 28, 2023
…er (#1351)

* Add the ability to supply multiple library paths in SearchPathContainer

* OpenAL: Change `soft` parameter from exclusive to preference

This means that now if loading OpenAL soft fails, ittl try to fall back to native, and if loading OpenAL native fails, ittl fall back to OpenAL soft

* SingletonSeparatedList -> SeparatedList

* Loader: Allow fallible loading of native contexts and libraries

* SilkTouch: Use fallible codepath for multiple libname fallback
Beyley added a commit that referenced this pull request Mar 29, 2023
…er (#1351)

* Add the ability to supply multiple library paths in SearchPathContainer

* OpenAL: Change `soft` parameter from exclusive to preference

This means that now if loading OpenAL soft fails, ittl try to fall back to native, and if loading OpenAL native fails, ittl fall back to OpenAL soft

* SingletonSeparatedList -> SeparatedList

* Loader: Allow fallible loading of native contexts and libraries

* SilkTouch: Use fallible codepath for multiple libname fallback
Perksey added a commit that referenced this pull request Mar 30, 2023
* Add the ability to supply multiple library paths in SearchPathContainer (#1351)

* Add the ability to supply multiple library paths in SearchPathContainer

* OpenAL: Change `soft` parameter from exclusive to preference

This means that now if loading OpenAL soft fails, ittl try to fall back to native, and if loading OpenAL native fails, ittl fall back to OpenAL soft

* SingletonSeparatedList -> SeparatedList

* Loader: Allow fallible loading of native contexts and libraries

* SilkTouch: Use fallible codepath for multiple libname fallback

* Update patch notes for 2.17 release (#1360)

* Make examples dispose windows (#1355)

* Load Assimp from source, rather than nuget in GL tut 4.1

* Make all OpenGL examples dispose their IWindow objects properly

* Make all Direct3D11 examples dispose their IWindow objects properly

* Apply overload exclusions to Vtbl functions

* Add DirectWrite bindings

* Add no-obsolete-enum control descriptor to Direct2D bindings

Any bindings after 2.16 should be using this control descriptor, as they arent released yet, and dont need to keep backwards compat

* Direct2D: Dont Generate IDXGISurface

* Add DirectWrite to solution

* Make d3d9 use the D3DColorValue from DXGI

While this is a breaking change, it effects approximately 0 people

---------

Co-authored-by: Dylan Perks <[email protected]>
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.

Multiple library names

5 participants