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

separated linalg module into a package #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

extrawurst
Copy link
Contributor

i have simply split up the module, no other changes for now.

  1. i think the linalg module was huge
  2. it is transparent since i created a package.d for the new package
  3. this makes it possible to implement alternative matrix-memory layouts without impacting the other types (see column major layout for matrices ? #66)

what do you think ?

@Dav1dde
Copy link
Owner

Dav1dde commented Nov 16, 2015

  1. It was relatively big, but not too big that it required a split imo.
  2. I wonder if there are corner cases, where it doesn't work (as basically everywhere in D)
  3. In what way would that help?

@extrawurst
Copy link
Contributor Author

  1. 2,5k file size with three totally different objects, at least violating single-responsibility rule on module basis is too big in my opinion. and mono-d had a considerable hickup on my machine opening that file.

  2. well at least the unittest do run on my end. also my projects all run fine using the new structure. even if there is a corner case that breaks there is the issue board for that iff we agree that this structure is desirable.

  3. I was thinking about implementing the other layout in another source file and import in the package depending on the specified (or default) version provided. and if not that there is still the limited focus since these layout topics just concern the matrix object.

what is you objection besides the possibility of introducing bugs, even though I did not change any code?

@Dav1dde
Copy link
Owner

Dav1dde commented Nov 16, 2015

  1. Well, I would blame Mono-D for that.
  2. I don't think it should break any code, was just interested if there maybe are some known bugs.
  3. Mh, I see, but I don't want to provide a matrix implementation for row major and column major. It's basically more or less useless code duplication. The decision I made when I started gl3n was maybe bad, but it is too late to change now.

I am not against it, just considering options. E.g. this change would break backwards compatibility e.g., this change breaks with older DMD versions. But I don't see a general advantage (except maintainability).

@DrInfiniteExplorer
Copy link

There are no obvious drawbacks in accepting this pull request, right?
Proper separation is a good thing.
The discussion about column/row is a separate issue, and to not impact the decision to go forward with this PR.

@Dav1dde
Copy link
Owner

Dav1dde commented Apr 22, 2017

I still don't see a benefit in splitting it (yet). If there will be a split in the future it will also involve changing more files, I really don't like that ext module etc.

@extrawurst
Copy link
Contributor Author

what do u mean by "ext module"?

@Dav1dde
Copy link
Owner

Dav1dde commented Apr 23, 2017

gl3n.ext

@DrInfiniteExplorer
Copy link

Ah, that gl3n.ext entered the discussion pretty randomly. Thought it was something related to these changes, was wondering why I couldn't find it in the changes for this PR.

Anyway.. what is the plan/vision then?
If you can make a description of what your plan is going forward, maybe we can help with progressing that plan

@Fr3nchK1ss
Copy link
Contributor

This change would be great. From my personal XP:

  • gl3n would get more views from people looking for a vector implementation, or matrix or quat (think Google search). The first time I checked gl3n, I thought the vector / matrix / quat would be imported from another module. I was going to drop the lib as I was looking for a vector struct. But the next day I checked again, as I could not imagine from the README that vector was not implemented. And I outguessed linalg as linear algrebra including vectors, not obvious, particularly for self-made dev with no math background (kudos to them). Personally, I thought linalg.d was an algo module of sort.

  • After some work with the lib, just searching through linalg.d is confusing, ie searching the keyword "rotate" for vector cycles through all rotate functions for matrices... Sometimes, looking up a member func, one does not know if it's part of Vector, Matrix, Quat... For example if I add a quick unittest for some reasons, I may write it below the wrong member func because tired/focusing on something else. When I made a change say 3 days before, it appears on my git status, but I must scroll through the diff to know if I modified vector or quat. The commit does not show straight away what the change is about. And so on. Those are practical difficulties I had, not theory.

  • Considering package.d works great for several years, as @extrawurst said the modification should be transparent in most cases. I would add that old D projects will have bigger deprecation problems than an external module change :) If the users of gl3n configured their project properly, the right gl3n tag is imported and new changes won't affect old code. If they used the infamous ~master, well, the use of GIT branch is indeed deprecated for dub!

I can do it, just need the green light. Also DServer under VisualSC gives tons of warnings, it would be nice to have clean cut modules to work those warnings incrementally.

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.

4 participants