BRL.Geometry

[HurryStarfish]

Recently, BRL.Geometry has been added. Having standard types for vectors etc. is nice and will surely be useful. :slightly_smiling_face: As I had made my own personal vector types before (though with less functionality than these ones), I've taken a look at the module to see what could be improved in my opinion. These are my suggestions, loosely in order of importance:

• [x] 1. I'd recommend using Double rather than Float as the underlying type. Afaik there isn't much of a reason to prefer Float over Double these days, unless memory consumption is a concern. Floats don't seem to have a no reliable speed advantage on modern machines (on x86 at least, operations on doubles can be as fast as or even faster than on floats, although caching-related effects can work in floats' favor due to their smaller size), so using doubles to take advantage of their much higher precision should be preferrable. Alternatively, a perhaps better solution, actually: multiple vector structs for different underlying types: Vec2D for doubles, Vec2F for floats (maybe even Vec2I for ints etc.).
• [x] 2. Simple numeric types like this, and structs in general, should be immutable (thread-safe, less potential for bugs, simpler to understand). They're mostly already built like that; just needs ReadOnly modifiers added to the fields and the []= operators removed.
• [ ] 3. Operator = definitely should do a component-wise =, nothing else. Making it an "approximately equals" check violates the principle of least surprise - = does an exact comparison for Floats and Doubles, so it should do the same for vectors of them. There could be a separate method instead, such as Method ApproximatelyEquals:Int(b:SVec2, epsilon:Float = 0.00000001).
• [ ] 4. I would not expect Operator* to do element-wise multiplication. The cross product is probably more commonly used. But operators should be as intuitive as possible, and both of them aren't really a good match for the symbol. If anything, I'd say the dot product fits best. It also has the advantage that it cannot be confused with either of the other two, since the return types differ.
• [ ] 5. Instead of Perpendicular, maybe a set of Rotate90/180/270 methods, along with FlipH and FlipV?
• [ ] 6. A general Rotate:SVec2(angle:Double) method would be useful.
• [x] 7. Suggestions regarding method names:
  • Lerp is not a very... intuitive name. Why not Interpolate?
  • Angle might be renamed AngleTo? It has a parameter, so imo this is more descriptive and reads better.
  • Same goes for some other methods, like Distance.
• [x] 8. @GWRon commented on the commit, saying that the name BRL.Geometry might not be optimal. My suggestion: how about splitting it up into BRL.Vector/BRL.Matrix/BRL.Quaternion? More modularity can't hurt, and I'd expect most of the time one wouldn't need the full package - for example, vectors are much more commonly needed than quaternions.
• [x] 9. I'd prefer the result of ToString() to be enclosed in something... "(1, 2, 3)" rather than just "1, 2, 3", or better yet, "SVec3(1, 2, 3)", to tell it apart from any other struct that might also have 3 elements (a simple 3D point, an RGB color, etc.)
• [ ] 10. On x64, having the operations implemented via SIMD intrinsics might be nice for improved performance. (Downside: overhead for storing the intrinsic type in the struct und unpacking its values into the regular, public fields after every operation - is it worth it?)
• [ ] 11. Regarding operators like Method Operator+:SVec2(b:SVec2) etc.: I suggest that standard BlitzMax-NG convention should be to put a space between Operator and the symbol. This might be different from how it's commonly done in C++, but it's more syntactically consistent in BlitzMax:
  • Operator is a keyword and acts as a modifier for Method; + is the method name. So the declaration syntax Method Operator + is analogous to Field ReadOnly x (and perhaps, sometime in the future, other cool things like Function Async DoStuff).
  • The space is required for non-symbol operators anyway: Method Operator Mod can't be written as Method OperatorMod
• [ ] 12. If there's a Method Operator -:SVec2(), there should be a Method Operator +:SVec2() as well... just for consistency sake, even if it's useless.

I can make a pull request out of this with the respective changes, but before that, I made this issue and the above list for discussion purposes :)

[GWRon]

Re 7. I would say "Interpolate" is too common. There is a set of often used interpolation functions (bounce, quad,...). This one here means "LinearInterpolate". Still "Lerp" is how many name it...so I am OK with it.

Re 9. I think type or struct names should not be part of ToString. Also brackets are only needed if you passed multiple triplets to the string.

ToString() is there to create a textual representation of the value behind the type. For integers it is the number, for strings the text, for rgb the three values and so on. You can always wrap some stuff around the ToString() call.

Think it depends on your assumptions around this command.

[HurryStarfish]

ToString() is there to create a textual representation of the value behind the type.

I agree, but that value is "a vector", rather than just "three numbers", so that should be indicated in some way. I guess you're right, having the type as a prefix might be in the way when you don't need it, and it's easier to add to the string in post than to remove it. But I'd say at least parentheses should be included. "(1, 2, 3)" or similar is also how you'd commonly write them in math.

[woollybah]

I've implemented some of the above.

@ SIMD I don't see much mileage in applying anything to the code as it currently stands. However, if we were at some point to introduce methods that process arrays of points then yes, I could see it might be useful. Otherwise, for a single matrix, for example, I doubt there's a significant efficiency to be found by implementing it using intrinsics.

Of course, if someone were to prove otherwise... :-)

[GWRon]

Won't gcc optimize stuff for us anyways?

[HurryStarfish]

I've added checkboxes to the items in the list to show which ones have been implemented.

An comment to 1.: Since there's SVec2F and SVec2I for Float and Int vectors now, I'd name the Double version SVec2D to match them. It makes the naming scheme more consistent and leaves open the possibility of adding something like Struct SVec<T> Where T Extends INumber in the future.

[woollybah]

I'm also working on some other related things - so far I have SBoundingBox - but I expect to have others. Any recommendations as to where to put these?

[GWRon]

If there were multiple levels for modules you could sort stuff better.

If it is auxiliary stuff then maybe use a new module-parent: math.mod/boundingbox.mod -> import math.boundingbox

[woollybah]

Indeed... I may as well move them all to math.mod then.

[GWRon]

The main module name is only used for categorization ..and to somehow define what is auto-imported if you do not use the framework command.

People who learn the language or do not want to remember where stuff was "sorted in" just want to do

Local v:TVec2i = new TVec2i

So if this is an often used type it should be in the list of auto-imported modules (like BRL for now).

This all would be not that important if the IDE would give the dev a hint..that a planned-to-use type is not known yet but defined in the following modules: Brl.vectors MyMathModules.vectors ...right click contains "add import statement".

Maybe an intermediary step is to add simple lookup support/cache to BMK. If a type name is not known during a compilation try , then BMK scans the module folders, creates a list of types...and puts a suggestion next to the error message. Yes it is not the task of BMK so maybe use it to write a little tool doing this (offload task) and BMK just asks the tool for a list of candidates to a given type name.

[woollybah]

I think I may move the new modules into their own namespace... I'd rather do it before the next release. If they are in their own namespace, it makes it easier to add new related functionality without adding lots more stuff to brl. math seems like a reasonable namespace identifier. This would also become part of the release, and could also become a default module.

[HurryStarfish]

BRL is indeed starting to become bloated as more and more stuff is added to it. But on the other hand it also feels weird to have something like Math on the same level as BRL and Pub. Seeing how those "main categories" normally represent the module author (or at the very least huge projects encompassing many smaller modules like wx)... A "cleaner" solution might be to allow for deeper nested namespaces as module names: BRL.Math.Vector, BRL.Math.Matrix, and so on. This would allow for deeper structure and finer separation, which would be useful for other new modules too (BRL.Collections.LinkedList etc). If somebody wants to bulk-import all modules below a certain namespace, there could be a syntax for that (Import BRL.Math.* perhaps). Not sure how much effort it would take to implement this, though.

[GWRon]

I think (and already proposed) a multi-level namespace would suit better when stuff has to belong to "default runtime modules" - of course it can create hassle if you add stuff to brl.math.XYZ which is not needed as default - so you add it to supplementary.math.XYZ which isn't that clean either.

Also we should not forget that all this "auto import/default modules" is only needed as the IDEs are not of help here. in Java the bigger IDEs tell you what to import and a right click later the import statement was added.

@ BMK and efforts Brl.MaxUtil offers some functions regardings modules. ModulePath() does replace "brl.math.vector"-path to something like "brl.mod/math.mod" and the ModuleIdent() only takes care of the last part: so ident is "vector". It looks as if BMK could handle all the stuff already. It's BCC which would need some love and attention.

[woollybah]

Well, I don't see a re-engineering of module hierarchy being done in the foreseeable future.

[GWRon]

You might have a look at the other issue I opened for thes regards.

Am sure it needs tackling some more stuff (mangled name clashes or so).

[HurryStarfish]

Well, I don't see a re-engineering of module hierarchy being done in the foreseeable future.

Ah, just to clarify, I'm not trying to suggest rearranging and of the existing, old modules. Just that the new, still-work-in-progress modules (such as Geometry and Collections) and future additions would benefit from being allowed to have nested namespaces.