-
Notifications
You must be signed in to change notification settings - Fork 642
Slimming down SharpDX (The main assembly, not the project) #398
Comments
Hi @PathogenDavid, Thanks for your feedback! Indeed almost everything you are referring have been around for some times (some parts were in skype/email discussions with @ArtiomCiumac, other part are well known long standing issues I have in my head) and were actually partly scheduled for this year, just that we hadn't time to start this large refactoring or even share them here, so if you are willing to help with this refactoring, I would be glad to bootstrap the process. Your proposal is welcome! Also we had plan this year to make SharpDX to be PCL compatible, so it is a lot related to this large refactoring. For the background, I agree that SharpDX has grown a bit outside its initial boundaries. Lots of the things that are in the library also came from the design of SlimDX, mostly because I wanted to have some backward compatibility with this library, and It helped also to bootstrap the project (like the SlimMath port) Though SlimDX was a big fat assembly, I wanted to split and decouple it. That being said, I hate to manage an additional assemblies just to decouple things to store just 3-4 classes in this new assembly, so if we could avoid such a case, I would be happy. The Toolkit also has brought some unnecessary code to the core assembly (serialization and some utils maths). The Multimedia is not ideal. The Direct3D is not a huge issue here, as it is only 2-3 classes, but could indeed be moved into a common Direct3D assembly...etc. I could discuss of this for hours, so I'm glad that you have an opportunity to help here. At Silicon Studio, we have copied back the math classes from SharpDX into our own assemblies (a
Yes. I had great projects for Math (mainly to bring some native math builtin interops to speedup things) but lack of time has not helped me to push things here. There is some consideration to later integrate the next .NET SIMD into the Math assembly, so this is something that is going to require some refactoring when .NET SIMD will be really ready. So overall, I agree that if we can make the separation, that would be better. I don't mind even to change the namespace. There are a couple of things that could be done to avoid even using
I agree. The serialization was a quick hack to be able to serialize data in the Toolkit, so we could move classes in the SharpDX.Toolkit assembly. Unfortunately, the design was a bit dirty from the beginning, so there is some strong coupling between the type and the serializer (so all Maths for example are using it), but this should be manageable to have serialziers outside the types, and register them to a factory instead.
I'm not completely happy about this, because it would require to create a
Yes. There are some dependencies that we would have to remove (For example, D3DCompiler is using just a little decoder FourCC but we could remove this dependency).
Yes, though most of the methods are used not by the Toolkit but by the interop layer in SharpDX, but no problem to split them.
Hm, it is not strictly related to the Toolkit, so It should stay in its own assembly
Yes. The Toolkit is accessing some internals from core assemblies, so It might require to expose these internals (but it is fine). One thing is to check that all platforms are building fine. As long as you do this for all major refactoring, that will be fine. Also, everything should keep the entire history of git (using git subtree...etc.), so I would probably prefer to setup a branch with an initial rough cleanup of assemblies so if you want to get into the details, you will be able to help there. What do you think? |
All exposed ideas are great and I agree with them, however this is a huge breaking change, so I propose to implement it in SharpDX |
@ArtiomCiumac, sure, I was not expecting to put it as a minor version! ;) I have just started to setup the split of assemblies first and will push this on a branch in the following days. It will not necessarily compile, but from this, we will be able to focus on the different parts. |
True, true. After experiencing your Mixed Platforms issue first hand, I can't say I blame you for not wanting to add more assemblies than necessary.
Yup, I am pretty excited for that! No more hacks that break when Microsoft updates (I dunno why I am linking this considering you made it...)
OK, cool. I wasn't sure how much of a concern backwards compability was.
Yup, this was the type of solution I was leaning towards for those. My only concern was it might make the API more confusing to use. The other route I was thinking is make the code that is currently pre-written generated as well, but that might make it a pain to edit since T4 templates don't get syntax highlighted, etc in Visual Studio.
Its not super ideal, but maybe dumb container structs of Float3/Float4/etc and forcing the consumer to implement implicit operators would be the easiest. There's probably something else though, I've not given this one much thought.
That sounds good to me, I did a little (manual) dependency analysis and found that Direct3D9 is one of the worst offenders in math usage: https://skydrive.live.com/redir?page=view&resid=798F537493B1975E!4031&authkey=!ANuki2UKIKiVb_M Unfortunately, Direct2D1 is pretty bad too.
Yeah, this is something I wanted to do as well.
Sorry, my wording was bad there. I wasn't referring to the "Utilities" class, I was referring to stuff like TimerTick and SingletonString.
Yeah, that'd make more sense.
I have a branch going on over in my personal fork: https://github.com/PathogenDavid/SharpDX/commits/sharpdxdiet Although it is pretty rough since I was mostly focusing on seeing "What can I remove and have this still work?", so I'd be OK with re-doing some of the stuff from there on a different branch to keep a clean history. (In particular, I completely mangled the poor math code.) @ArtiomCiumac |
I started modifying SharpGen to produce output that uses generics instead of hard-coded SharpDX Math types. Its not 100% rock solid/tested and should probably get a little cleanup, but it works for removing the dependency on ViewportF and Rectangle by Direct3D11. It also needs to emit a typeparam documentation element still. Here is a sample of its output:
The change to the mapping XML looks like this:
EDIT: I forgot to mention. The size that TViewportF will be checked against is grabbed from the My changes are live in my sharpdxdiet branch, but do be warned that I've only tested with Debug|Net40 and all only the Direct3D11 project enabled. It might break if other bindings try to build right now since they haven't had non-generated code updated. Let me know if either of you have any questions or concerns with how I've gone about this so far. |
Great! I will have a look as soon as I have setup the new branch |
I have tested some split of the Matthematics, Direct3D and Multimedia, and move some types to the Toolkit (serialization, components, collections...etc.). Size of assemblies are:
Before this, SharpDX was alone around 592Ko, so considering that Direct3D and Multimedia are very small, I will probably let them inside SharpDX, as it seems not really worth to bother a decoupling. |
Hm, as I feared, splitting Mathematics is not easy. For example, in DXGI, some simple structs are using SharpDX.Rectangle, or SharpDX.Color4.The problem is these two types have some properties and methods that would pull more structs from Mathematics. We could define some generic types in SharpDX, like kind of Int4, Float4 that would not have any methods. But then in SharpDX.Mathematics we would like to provide automatic casting between these raw types and Rectangle/Vector4/Color4 for example... I would prefer SharpDX.Mathematics to not have a dependency to SharpDX, but it seems difficult... Ok, so what could we easily move to SharpDX.Mathematics? All Ray, Collision, boundingbox, Plane, Angle...etc. stuff. But It seems that moving types that are used for some interop scenarios (Vector4/Color4/Matrix...etc.) would cause more pain when using SharpDX... EDIT: we could still use generic for method parameters but for struct members, this is really not possible as explained above... So, what do you think @ArtiomCiumac and @PathogenDavid? |
What methods are required from
This will allow decoupling from Math implementation, while still providing some more behavior than just data transfer. |
@ArtiomCiumac the problem is less for parameter marshalling in methods (that can be solved with generics) but mainly with struct members. There are a couple of structs around that are using |
Ok, I see the problem. I think we should leave these structures in As regarding to another structures - I am not sure if added benefit outweights the increased complexity (I mean usage of generics). |
I will probably keep standard types in SharpDX (VectorX, ColorX, MatrixX...etc.) and move all collision/angle/bounding box stuff to a Mathematics assembly. It seems to be the only practical split without duplicating types and will avoid too much confusion when using SharpDX. |
…classes only used by the Toolkit to SharpDX.Toolkit WIP (#398)
I have pushed a new branch diet with an initial commit (18d89eb) that contains mainly:
Results so far are not huge as |
Call it
The |
Sorry for the lack of comments on this yesterday, had a particularly long work day.
Sounds OK to me. I mostly mentioned them as something I saw that could be separated out, but if they are impacting things so little then its probably not worth having an extra assembly floating around.
Yeah, this is certainly troublesome. I had not noticed / considered the depencencies from the generated structures.
Are you referring to using generics for the interface classes?
PixHelper is also only relevant for DX9, right? Or does Pix still work for DX10/DX11? I was under the impression it was replaced with the Visual Studio graphics debugging tools for modern DirectX.
I definitely agree with this. It would also prevent confusing when reading documentation / browsing around in Intellisense. I know I've been bitten several times when exploring a new API and finding that exposed types / methods / whatever sound like what I want but are only relevant for a part of the API I'm not using or are only useful internally. @xoofx |
It doesn't mean that it is not used outside the Toolkit! ;) We are using it at work without using the Toolkit. As it can be used on Direct3D9 as well as Direct3D10 and Direct3D11, hence the reason it is in SharpDX assembly.
|
Would it make more sense for PixHelper to be in SharpDX.Diagnostics? |
We can do it for method parameters, that's fine (And only in recent core assemblies, like Direct3D11).
If you meant the SharpGen changes, yeah sorry, I just had a quick look. Still not sure it is worth the additional complexity. Some of the methods which can take generics are mapped to IntPtr already, so they can be extended by some handwritten methods. There are only few methods that would require this, so not sure about the benefits... |
Quite crazy idea, but... What do you think about making a mechanism that would allow linking SharpDX to a math library by the end-user? The C++ headers are really needed just initially, after that - the intermediary data (output of GccXml) can be stored somewhere and the user can build the needed assemblies himself by defining the mapping dictionary (something like This could be a "special" build of SharpDX which will not require the native headers as everything will be already prepared for generation and compilation. This will not require any generic "magic", while allowing anyone to link to his own math library and preserving the "wrapper-only" state. Quite crazy though... |
@ArtiomCiumac |
With the "generics" route we hit the situation when some structures need to be used during code generation phase as members of other structures - and we can't use generics there. My opinion is that using generics in some areas, while in others - our own structures will lead to confusion. |
Hmm, that is definitely true. I was going to say we could allow generics to be toggled for those that need it (a fairly simply change with the way I have it implemented), but I guess that isn't much different than allowing people to use their own math library like you suggested. |
Hm, if someone want this kind of customization, It can already do it by modifying mapping.xml files. But I really don't want to introduce such a customization. We have been using SharpDX in Paradox without any problem even using our own math library. There were only 2/3 calls to cast from our math lib to SharpDX lib (ClearRenderTargetView...etc.). So it is really not a huge issue... But, after reconsidering it, I'm going to look how much exactly pure basic types we need for interop. Probably just a few, like RawRectangle, RawFloat4 (without any methods in them, just simple structs) so if we are making SharpDX.Mathematics dependent on SharpDX, with implicit operators conversion that would be almost transparent for users and unnoticeable in terms of perf (as there are no critical methods/structs for these cases in SharpDX). This would slim down SharpDX assembly and allow a real SharpDX.Mathematics to evolve on its own (I would accept the dependency to SharpDX) The SharpDX.Mathematics would be used by the Toolkit and it could even have its own repository. I will try this in the following days. |
Sounds like a plan. Any way I can help? |
Of course! Though during this initial split it is difficult to work concurrently on it, but right after, there will be lots of opportunities to help:
I will be busy for the next 2 months and probably won't be able to work on all these issues after the initial split. So any help with these is welcome |
Ok, I have pushed the changes and added new raw types in SharpDX.Native namespace (if you find a better name?) and added implicit converters to SharpDX.Mathematics. The toolkit is not compiling. I have also only tested compilation on net40 so it might not work in other config. |
Awesome, I will take a look over it when I get home today. I'll also look at your earlier list and start working on some of those things.
I would think |
@PathogenDavid I have renamed to As I said earlier, I'm going to be busy for the next weeks but feel free to open pull-request on this branch and I will have a look at them. |
I'm thinking it may be. I've spent this morning working on fixing the toolkit and ran into the issue that anything that takes arrays. For example, EG: SharpDX.Direct3D11.RasterizerStage.GetViewports doesn't work with an array of ViewportF even though ViewportF casts to RawViewportF. So either SharpGen needs to expose a way to pass the array as a void pointer, or it needs to allow generics so that the manually implemented code in DeviceContext.RasterizerStage.cs is able to take in different types of arrays. |
This issue for arrays should be fixed by previous commit. Modifying SharpGen is fine if there are lots of method candidates for generic parameters, otherwise manual changes are fine. Moreover when we need to provide methods supporting single argument and array varargs, it is easier to do this manually. |
…d usage of old math structures by their Raw* equivalents (#398).
…ing old structures by their Raw* equivalent (#398).
… structures by their Raw* equivalents (#398).
In the commits above I have fixed compilation on all platforms. |
Hey all! Sorry for the radio silence, I've been unable to have time for this project for various reasons the past month, but I'm back to help again! Before I got lost in the jungle, I had started working on splitting the toolkit away from the main repo. My interpretation of "Migrate the Toolkit to another repo (with preserve commit history)" was that you wanted a repository with only toolkit stuff in its history. I tried several solutions involving various forms of I also looked into git sub-tree, but it didn't seem like an ideal solution since the Toolkit stuff isn't completely isolated into one folder. I think this could work though if we are OK with maintaining the history for things within Source/Toolkit, and loosing it for everything else. The other alternative is that the history of everything else non-toolkit gets lugged around with the repository. Its not ideal, but it is certainly the easiest solution. Here is the SharpDX repository with only things I thought were necessary for the Toolkit: https://gist.github.com/PathogenDavid/0a0a8443c6179cc33a04 (This may be out of date to any changes made since this commit.) So these are the options as I see them:
Any thoughts or preferences? I don't like lugging around the extra unnecessary history, but #1 might be the ideal solution. |
Thanks @PathogenDavid I won't be able to follow this closely as I'm on vacation for a few weeks. Also, not entirely sure if I would like to duplicate some build files (or even SharpCLI) between the repos. Ideally, It would be better, but as we have to maintain/recompile/distribute them, I would prefer to ease our task on this. I'm thinking that It would not be a huge issue if the Toolkit repo had a dependency on the main repo (sub-module), so that we are able to reuse build files and integrate SharpCLI directly into the toolkit sln. May be there wil be some relative paths problems with the build files, but this should be fixable. There are some downsides with this solution (direct dependency with internal builds, maintain submodule...etc.) and discrepancy (ideally would be easier to have a dependency on nuget packages instead) Lastly, there are some issues to consider on how the Toolkit will be distributed (dependency to main assemblies) and how it is going to affect nuget distrib. Though not a huge problem, so we will have a look later at this. |
Hello there! I have pushed a few more commits into diet branch. A new Also I have moved the |
Closing this issue, as the latest master is integrating the diet branch for SharpDX. I will fix the Toolkit repo later. |
First, I apologize if this has been discussed before or there is already an ongoing effort to do what I am describing. I could not find anything related to it.
Second, I am mostly posting this because I am making these changes to a personal version of SharpDX anyway. I thought I should start a discussion to determine if I should make my changes in a manner that allows them to be integrated into the main SharpDX repository at some point. I realize that what I am proposing is somewhat of a breaking change, but I thought I should say something in case Alexandre is already working on something similar at Silicon Studio or there is a general interest in this being done.
SharpDX advertises itself as "an open-source project delivering the full DirectX API under the .Net platform". While SharpDX certainly accomplishes this goal, it also comes with a ton of utility code to facilitate easier development of graphical applications. Most of this is the SharpDX Toolkit, but there is still much of it in the main SharpDX assemblies.
This unnecessary code seems to come from three primary locations:
As an example, the following files are unnecessary for building and using the SharpDX bindings:
There may be more, but these are what I found while slimming down SharpDX for my own project. I more or less determined this through a combination of Visual Studio's "Find all references" tool, and removing+re-adding things while building for every target platform. I also removed much of the inter-dependencies between math library bits and anything using the serialization simply for the purpose of making things bare bones.
My proposal would be to:
My personal primary motivations for this are:
I guess the overall goal is to make SharpDX more of a pure DirectX managed binding, rather than the "DirectX managed binding (And more!)" that it is right now.
Once again, I am not posting this issue because I am trying to tell you how to maintain your library. I am posting it because if it is something the primary maintainers desire, I am willing to put more effort into "doing it right" rather than making my own custom fork of SharpDX.
TL;DR: The main SharpDX assembly nets you a lot of unnecessary cruft, I am reducing it for my own purposes and want to know if there is interest in this being something done in the main repo.
The text was updated successfully, but these errors were encountered: