It should not be possible to use a getter as a setter

A getter should not be used as a setter

True, but the access to private member is worse. Also it's common practise for our code. So it'd be good to improve it. But it's not possible here yet without much useless code, until we have callbacks for configs.

Private member access bad because it's the responsibility of the class that owns the member to set the member, therefore it should be CGameViewImpl loading it's own data, rather than CGameView loading data and pusing it into CGameViewImpl and the calls should be moved, and the getters can be const if they are needed at all? I didn't check if the CGameViewImpl / CGameView separation is useful to begin with.

This moved hunk would be better off in a separate patch that doesn't change the logic, then one can read the diff that changes something independent from the diff that doesn't change logic. This move looks legit at first sight.

good

This would be be cleaner to use the Vector2D <-> JSValue conversion from GuiScriptConversions.cpp.
(Needs equivalent change for loading then)

I honestly haven't ready anything on where this gets called but returning a reference to a float should basically never happen anywhere in code. A pointer I might accept because at least it's obvious that it's a pointer when used elsewhere but a reference is just too dangerous.

True, the only reason why I added this - many useless code to add. It's used in the following lines (6 times):

CFG_GET_VAL("view.pos.smoothness", m->PosX.GetSmoothness());
...

So if we'd use getters/setters, we've to add an additional code for each line like:

bool smoothness;
...
smoothness = false;
CFG_GET_VAL("view.pos.smoothness", smoothness);
m->PosX.SetSmoothness(smoothness);

Or use the a macro.

A pointer I might accept because at least it's obvious that it's a pointer when used elsewhere but a reference is just too dangerous.

I agree with pointer.

Probably it'd be good to process this patch after D1573.

Some sort of CFG_GET_VAL_AND_SET macro seems like the obvious better option here to me - but agreed.

Or CFG_GET_VAL_OR_DEFAULT :)

2019

2019

If you are going that road -> Doxygen style

/**
 * Unloads all graphics resources loaded by RegisterInit.
 */

arguments should be const ref unless they are intended to be changed or unless we need a copy, right?
There have been debates whether a reference to a number is faster than copying the number, but it seems good to have it consistent and more logical to me to avoid allocation of a new variable if we can avoid it.

Perhaps the implementation can store the Vector3D directly, so that we can return a const ref here instead of creating a vector? (I didn't check)

I guess this was only sorted by public / private

The GetCameraX,GetCameraZ -> GetCameraRotation change is good

This should pass the Vector3D directly, should not be much to rework

Basic types like float, int as so on (not double or long long on 32bit platforms) may but don't have to be passed as const ref, because they fit into usual CPU register. There is no allocation. Because all these variables will be created on stack.

Example:

class Test
{
    int m_Value = 0;
public:
    __attribute__((noinline)) // disable inlining
    void foo1(int value)
    {
        m_Value = value;
    }

    __attribute__((noinline)) // disable inlining
    void foo2(const int& value)
    {
        m_Value = value;
    }

    int GetValue() const { return m_Value; }
};

Test test;
int g_Sum = 0;

void foo(int n)
{
    if (n > 0)
        test.foo1(n);
    else if (n < 0)
        test.foo2(n);
    else
        g_Sum += test.GetValue();
}

Clang with -O0 assembler (rdi = this):

Test::foo1(int):
        push    rbp
        mov     rbp, rsp
        mov     qword ptr [rbp - 8], rdi
        mov     dword ptr [rbp - 12], esi
        mov     rdi, qword ptr [rbp - 8]
        mov     esi, dword ptr [rbp - 12]
        mov     dword ptr [rdi], esi
        pop     rbp
        ret
Test::foo2(int const&):
        push    rbp
        mov     rbp, rsp
        mov     qword ptr [rbp - 8], rdi
        mov     qword ptr [rbp - 16], rsi
        mov     rsi, qword ptr [rbp - 8]
        mov     rdi, qword ptr [rbp - 16]
        mov     eax, dword ptr [rdi]
        mov     dword ptr [rsi], eax
        pop     rbp
        ret

Clang with -O2 assembler:

Test::foo1(int):
        mov     dword ptr [rdi], esi
        ret
Test::foo2(int const&):
        mov     eax, dword ptr [rsi]
        mov     dword ptr [rdi], eax
        ret

        mov     dword ptr [rsp + 4], edi
; foo1 call:
        mov     esi, edi
        mov     edi, offset test
        pop     rax
        jmp     Test::foo1(int)
; foo2 call:
        lea     rsi, [rsp + 4]
        mov     edi, offset test
        call    Test::foo2(int const&)
        pop     rax
        ret

As you can see the const int& contains one more memory reading. Also compiler still optimised calls and used registers directly despite the disabled inlining.

In most cases these operations are really near, but const int& can be a little slower, like by 1-2%, also there're cache misses.

But then we need to check and update it, when we changed the source.

I think it's irrelevant to the patch goal. As I mentioned below I suggest to create another ticket/task, as it's pretty simple but requires more changes.

It'd good to do so, I suggest to make it as a simple ticket/task for that for beginning contributors.

Thank you, I have never seen a that factual analysis on this!

I wonder if we could make this a coding convention to gain consistent code, to avoid repetition of these debates and to inform people of the reasons behind the omitted reference and for which fundamental types references should be used.

EDIT: See comment below, I assumed we have the Vector3D conversion too, but we only have the Vector2D conversion. If we had them, then I would argue:

Relation to the implementation:
If one adds a new line to the codebase, one should examine that the line is correct.
By proposing to write a ticket, you acknowledge that the lines are not the way we want them to become. So:
Choice 1: Replace 3 lines with 3 lines that you know to be wrong, create a ticket to fix that
Choice 2: Replace 3 lines with 1 line that you know to be right, and 1 modified line in session.js, not create a ticket

So it's not like we have the choice between investing more time to fix something additional, but it's saving time while fixing things.

It's arguably related to the goal of the patch, if the goal of the patch includes to use one CVector3D getter instead of three numeric getters above.

Arguably the argument could be extended to have SetCameraData use the vector. Then it would indeed cost a 2 more lines changed, but probably still less time than modifying the line once, then writing a ticket and modifying the same line again.

Anyway, you are free to chose, I'm confident you don't break things and it's easier to fix 10 lines of code than to argue in 30 lines.

But since we don't have the Vector3D conversion, the argument is dead.
I remember trying to write a macro once, so that we don't have to copy&paste it for 2D/3D, but I discontinued working on that, wasn't so easy.

That comment was wrong, we already have that conversion in ScriptConversions.cpp, it's used for JS <-> C++ entity positions for example.
We only don't have the Vector3D one I think

Yeah, notes in CC would be useful.

Yeah, my point for ticket was about adding the Vector3D conversion, not the just lines replacing.

Doxygen

Should be 0.f no ? Same for everything below.

Macro / Function ?

why no & ?

Should we really return int ?

Magic number ?

Magic number ?

Magic number ?

Could be a doxygen todo or a ticket

Should be doxygen.

Same here

Shouldn't that be at the top of the file ?

Still accurate :D

Because it's a reference, not a pointer.

I think we shouldn't, but it's not bad here yet.

Yeah, unfortunately we have magic numbers for RegMemFun. We need a task to refactor it, it also occurs in other files.

It's not todo I suppose, because we didn't want to use it AFAIK.

I thought this was already a rule we had TBH, not passing trivial types by reference is fairly classic C++ advice.