Overload operators

[ES-Alexander]

PR to implement #142.

Note: currently only includes an attempt at implementing __add__, and doesn't compile because error: non-virtual member function marked 'override' hides virtual member function for basically all the methods in domain.hpp

I clearly don't know much about what I'm doing here, but the process I went with was:

1. remove the definitions of all the methods in domain.hpp
2. put them in domain.cpp instead, and #include "domain.hpp" in there
3. add a function declaration for __add__ in DomainBase in domain.hpp, which returns a Translate instance and takes in the same signature as the Translate constructor
4. put in a seemingly appropriate definition in domain.cpp to perform that python operator overload

Definitely not in a mergeable state yet, but hopefully a step in the right-ish direction.

[nschloe]

Why separate domain.hpp into .hpp and .cpp?

[ES-Alexander]

I assumed it was necessary because otherwise the Translate class wouldn’t be declared before DomainBase tries to use it, but perhaps that’s an incorrect assumption?

[nschloe]

I assumed it was necessary because otherwise the Translate class wouldn’t be declared before DomainBase tries to use it,

It works in main without a .cpp, right?

[nschloe]

I assumed you'll just have to add stuff to DomainBase.

[ES-Alexander]

It works in main without a .cpp, right?

I have no idea - I just assumed I needed to split it so started with that. I’ll quickly try fresh with just the extra bit, but after that I need to get some sleep so will have to come back to this later

[ES-Alexander]

Nope, unfortunately errors out with unknown type name 'Translate'.

(All I tried here was just adding the add method to DomainBase in domain.hpp in a clean pull of main, then run build)

[nschloe]

Nope, unfortunately errors out with unknown type name 'Translate'.

I don't know what code you're running exactly but I can say it works in main.

[ES-Alexander]

I don't know what code you're running exactly but I can say it works in main.

I've added

  virtual
  Translate
  __add__(
      const std::shared_ptr<const pygalmesh::DomainBase> & domain,
      const std::array<double, 3> & direction
  ) const
  {
    return Translate(domain, direction);
  }

to the DomainBase definition. My branch is from main, so that's just a direct add to domain.hpp and the rest of the code/repo is the same.

[nschloe]

I think it should be operator+(), but this has nothing to do with the error you got. One way around this would be to put all logic into this function and have the Translate class just be a wrapper.

[ES-Alexander]

I think it should be operator+()

Very possible - I'm thinking in Python land since that's what I'm most familiar with, but it's possible it has to be the C++ operator overloading functions instead, likely worth looking into/trying both to see what works.

One way around this would be to put all logic into this function and have the Translate class just be a wrapper.

Interesting idea. If I'm understanding you correctly that would basically make the operations on a DomainBase instance into modifier methods of the underlying object instead of new classes. I suppose then that the current classes would need to be turned into functions instead, that basically just create a copy of the passed in DomainBase, run the desired method on it, and return the modified DomainBase result. That has the added bonus that if an old object isn't required after an operation it can be modified in place with no copying needed.

I'm unsure how the current method-stacking logic would be applied from an instance method though. Can a C++ instance method dynamically wrap another instance method? I suppose that has to be possible somehow, although I'm not sure how it'd be done. The Python equivalent would be

    def translate(self, direction):
        old_eval = self.eval
        def eval_wrapper(point):
            d = point - direction # assuming numpy array
            return old_eval(d)
        self.eval = eval_wrapper
        ... # similar for translate_features and get_bounding_sphere_squared_radius

but that's reliant on functions being first-class objects that you can just move around and reassign at will.

I'll have to have a play over the weekend, and maybe look into C++ a bit more if I've got the time. I feel like this is a decent learning experience, and useful functionality to have, just takes time... Unfortunately I'm wanting to use this in a project, so not finishing/implementing it asap means inevitable reworking for my own project later on, but I'll have to see how it goes.

[ES-Alexander]

I suppose it's always possible to create a specific eval method for each operation (and similarly for the other relevant methods), and manually create a function stack and object stack for the wrappers and the objects they need, and add to those stacks in the modifier methods (e.g. translate) then have the eval method recursively run through the eval wrapper function stack and object stack from top to bottom, which is equivalent to what the Python version would effectively be doing in the background. It would of course be preferable to not have to do that manually, and doing so would require the object stack to be untyped pointers that get cast to the correct type by the function using them, which could be problematic/painful...

[ES-Alexander]

Had the grand idea of creating a subclass of DomainBase after the modifier classes are defined, and implementing the operator overrides there. That way the various domain objects can inherit from DomainModifiable instead of DomainBase and life is good (I thought). Having thought about it a bit more I've realised that since Translate inherits from DomainBase, even if I could get this approach to work it would only do so for a single modification, after which additional modifications would require the existing process of directly using the modifier classes.

Before I had that realisation I attempted to implement a DomainModifiable class, which is what's included in the latest commit. My implementation attempt currently fails in trying to convert the subclass object (passed in using this) to the const std:shared_ptr<const pygalmesh::DomainBase> that the Translate constructor expects. I played around a bit with std::make_shared but don't know what I'm doing so didn't get it working.

I looked a bit more into nested functions in C++, which seems to be possible using lambdas, although I believe the 'dynamically wrapping an existing method' side of things might not be possible, so another approach is likely required.

I would rather avoid the stack approach if at all possible, because it's complicated to implement and understand. I still feel like this should be achievable with some kind of forward declaration of Translate, but I don't know how to do that since Translate inherits from DomainBase, and I believe I need to use Translate as a return type in the DomainBase declaration.

[ES-Alexander]

Latest attempt is using auto as the return type, and separating the declaration from the definition, with the definition put after Translate is defined. Tried some extra stuff I found to try to get the conversion of this into a shared_ptr working, but it hasn't helped, so currently stuck on the shared_ptr conversion problem.

On the plus side, compiling isn't complaining about Translate not existing anymore, so maybe auto will actually work for the return type. I've had to make the function not virtual anymore to enable auto-deduction of the return type, but I'm presuming that subclasses are unlikely to override these operator overrides anyway (hopefully), so not too concerned about that (my understanding of this is very limited and possibly wrong, but that's what I read virtual enables in my looking around).

[nschloe]

Is this still being worked on?

[ES-Alexander]

Hi @nschloe, the project I was initially planning on using this for is on the backburner at the moment, and I'm afraid I've forgotten if I was still planning to use this in it once that project resumes (I'm not sure when that will be).

Accordingly, I may still do some work on this in future, but don't know for sure, and don't know when that would be either.

I'll close the PR for now - I can always re-open it if I come back to it :-)