Coordinate system issues: cylindricalCS / fixedProfileFvPatchField

• cyclindricalCS cannot be run-time selected: missing TypeName and addToRunTimeSelectionTable.
• hard to specify

        coordinateSystem
        {
            coordinateRotation
            {
                type            axesRotation;
                e3              (1 0 0);
                e1              (0 1 0);
            }
            type            cylindricalCS;
            origin          (0 0 0.01);
        }

With this in place we could re-write various boundary conditions to use cylindrical coordinates. E.g. fixedProfileFvPatchField.

@matej

Remark: cylindrical coordinateRotation currently stores transformation tensors. This should be stored outside.

Or have Function2, Function3 and directly obtain a value from a (set of) 2D/3D coordinate(s).

This could ultimately replace e.g. timeVaryingMappedFixedValue.

changed the description

changed title from fixedProfileFvPatchField is 1D in cartesion coords only. Why not e.g. axisymmetric to Coordinate system issues: cylindricalCS / fixedProfileFvPatchField

changed the description

added bug label

coordinate system cannot be re-read. It requires keyword coordinateRotation which does not get written. Writing gets done with:

    os.writeKeyword("coordinateSystem");
    coordSys_->writeDict(os, true);

• type gets written as coordinateSystem instead of cylindrical
• there is no coordinateRotation section

coordinateSystem::New(const dictionary&) doesn't use the run-time selection system so is only good for constructing a coordinateRotation.

While fixing #884 (closed), I started taking a more general re-visit of coordinate systems. I have a local branch with various bits of refactoring that will cleanup a fair amount of other junk.

I also noticed the output you mention (which really does look quite dubious), but held off making any radical changes at all for 1806.

I believe that the coordinateSystem::New(const dictionary&) is a holdover from when the globally referenced coordinate systems got put in and we changed to the two-parameter form. At some point, these were also supposed to work with having type being optional and defaulting to the equivalent of cartesian.

More: coordinateSystem has non-virtual clone(), writeDict(). clone() uses default copy constructor so does not copy coordinateRotation (since autoPtr steals it). Hence cannot be used in e.g. bc in decomposePar.

This is one of the issues that I detected with autoPtr. This is the main reason std::auto_ptr was deprecated and replaced with std::unique_ptr that only allows a move construct and move assignment.

If we undefine these

// Transitional features/misfeatures:
#define Foam_autoPtr_copyConstruct
#define Foam_autoPtr_copyAssign
#define Foam_autoPtr_castOperator

we'd see that things like coordinateSystem etc have default copy constructors, which implicitly call the autoPtr copy (and steal) constructor.

IMO stealing should be disallowed, but this means that all classes that currently have an autoPtr as a member will to explicitly define a copy constructor.

mentioned in commit 8a9db156

mentioned in commit f34edc62

mentioned in commit 1a7cc714

mentioned in commit 3c3cf51f

Conclusions from discussion yesterday with @Mattijs about !211 (merged)

1. The split of coordinate system into the origin (coordinateSystem) and orientation (coordinateRotation) isn't really need. We only need a runtime means of specifying the orientation. So move the rotation matrix to be a direct part of coordinateSystem and its associated transformations.
2. Position-dependent transformations (eg, a vector at some location in a cylindrical coordinate system) belong in the corresponding coordinate system (eg, cylindricalCS).

Discussion today with @andy.

• OK with the above points, but for things like rotorDiskSource that are called often, should provide a caching mechanism for the position-dependent rotation tensors. The caching may need to be realized via some fancy sub-class instance or equivalent with a capture of a given pointField reference. The current coordinateRotation has both R() and Rtr(), but we can eliminate one of these by simply using the opposite tensor-vector, vector-tensor inner product instead.

caching mechanism: can we see if it goes without. For most cylindrical transformations we don't need a tensor but can just use a crossproduct instead.

mentioned in commit 301ffe43

mentioned in commit 0d3bcd1b

mentioned in commit e338c11a

mentioned in commit 53637669