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

CBot class inheritance #540

Closed
Otaka opened this issue Jul 27, 2015 · 21 comments
Closed

CBot class inheritance #540

Otaka opened this issue Jul 27, 2015 · 21 comments

Comments

@Otaka
Copy link

@Otaka Otaka commented Jul 27, 2015

I have tested colobot version 0.1.5.0.
I thought that if cbot has classes, that means that should be inheritance of the classes, but I am not sure if it still in work or it is a bug, or I am doing something wrong because it does not documented.
I am trying the following code:

extern void object::New() {
B b=new B();
b.test();
}

public class A {
public int baseMethod(){
message("Base method");
return 2;
}
}

public class B extends A{
public void test(){
message("Child method");
int returnValue=baseMethod();
message("After baseMethod executing");
message("Return value "+returnValue);
}
}

The code is compiled without any errors but when it runs it shows the messages:
"Child method"
"After baseMethod executing"
"Variable not initialized"

Expected result:

"Child method"
"Base method"
"After baseMethod executing"
"Return value 2"

@krzys-h
Copy link
Member

@krzys-h krzys-h commented Jul 27, 2015

Class inheritance is not currently implemented in Colobot, but probably will be after CBot refactoring. The fact that this code compiled without errors is probably a bug or a forgotten attempt to implement it.

@krzys-h krzys-h added this to the alpha-0.2.0 milestone Jul 27, 2015
@krzys-h krzys-h changed the title Colobot class inheritance CBot class inheritance Aug 9, 2015
@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 13, 2015

Class inheritance works for class member variables but not for methods.

public class baseclass
{
    int aaa = 1;
    int bbb = 2;
    void method1()
    {
        message("method 1");
    }
    void method2()
    {
        message("method 2");
    }
}

public class extension extends baseclass 
{
    int ccc = 3;
    int ddd = 4;
}

extern void object::TestExtends()
{
    baseclass base();
    extension ext = new extension;

    ext.bbb = 5;//overwrites b in this class only, unless its parent is declared static 

                                                          // output
    message(" ext.aaa = " + ext.aaa  + "       ext.bbb = " + ext.bbb ); // 1    5

    message("base.aaa = " + base.aaa + "      base.bbb = " + base.bbb); // 1    2

}
@Otaka
Copy link
Author

@Otaka Otaka commented Nov 13, 2015

I think if it works for fields, that means that not working methods in inheritance, it is a bug.

@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 16, 2015

Parent class methods are referenced with the "super" keyword.

This compiles and works. Is this considered inheritance??

public class parentclass
{
    int x=128;
    void p_method(string caller)
    {
        message( "p_method called from " + caller );
    }
    void parentclass()
    {

    }
    void ~parentclass()
    {

    }
}

public class child extends parentclass
{
    static parentclass parent; //null reference (optional)

    void c_method()
    {
        super.p_method( "child" );       // call parent method with "super"

        parent.p_method( "child" );    //(optional)

        message("child says: super.x = " + super.x); //get parent var.
    }
    void child()
    {
        parent = super; //public reference to parentclass(optional)
    }
    void ~child()
    {

    }
}

extern void object::Inheritance() //--------------------------------Main-------------
{
    child kid();       //new instance of child

    kid.c_method();                  //child method can access parent method

    kid.parent.p_method( "extern" ); //parentclass by reference
}
@krzys-h
Copy link
Member

@krzys-h krzys-h commented Nov 16, 2015

THERE IS A SUPER KEYWORD?! It doesn't even get highlighted by the editor.
Anyway, I did some testing myself and it turns out it works almost perfectly:

public class TestBase
{
    int x = 1;

    void TestBase()
    {
        message("TestBase constructor");
    }

    void testPrint()
    {
        message("x = "+x);
    }

    void testNoOverride(string s)
    {
        // This method of the base class is not overriden in the child class
        message(s);
    }

    void testRecursion(int x)
    {
        message("TestBase::testRecursion called! THIS IS BAD"); // shouldn't get called
    }

    void testNoSuperCall()
    {
        message("parent, SHOULDN'T HAPPEN");
    }
}

public class TestClass extends TestBase
{
    int y = 2;

    void TestClass()
    {
        super.TestBase();
        message("TestClass constructor");
    }

    void testPrint()
    {
        super.testPrint();
        message("y = "+y);
    }

    void testInsideCallNoOverride()
    {
        testNoOverride("testInsideCallNoOverride"); // without super.
    }

    void someOtherMethod()
    {
        super.testNoOverride("this is TestClass::someOtherMethod");
    }

    void testRecursion(int i)
    {
        if(i == 0)
        {
            message("Recursion works!");
            return;
        }
        testRecursion(i-1);
    }

    void testNoSuperCall()
    {
        message("child");
    }
}

extern void object::New()
{

    TestClass test();
    test.testPrint(); // works correctly
    test.x = 2; // change base
    test.y = 1; // change child
    test.testPrint(); // works fine
    test.testNoOverride("This should work"); // THIS DOESN'T WORK. Doesn't throw a compile error or anything, it just does nothing with executed
    test.testInsideCallNoOverride(); // BROKEN, this is apparently also a no-op
    test.someOtherMethod(); // works correctly
    test.testRecursion(5); // this also works as expected (I was trying to relate this to issue #673)
    test.testNoSuperCall(); // this also works fine
    message("Finished without crashing!");

}

In short - if you call a method from a base class that is not overriden in the child class, it will just do nothing.

@Otaka A temporary workaround would be to add this to class B:

public int baseMethod()
{
    return super.baseMethod();
}

Also, @MrSimbax, this needs to be documented in SatCom

@Otaka
Copy link
Author

@Otaka Otaka commented Nov 16, 2015

yes))) It works. Thank you very much.
Found another case with inheritance that is not work(or may be I still did not find the way how it should be written) - Polymorphism.

public class MyAbstr{
    public void test(){
        message("Base test");
    }
}

public class A extends MyAbstr{
    public void test(){
        message("A method");
    }
}

public class B extends MyAbstr{
    public void test(){
        message("B method");
    }
}

extern void object::New(){
    A a=new MyAbstr(); //(Expected: Should not compile) Actual: compiled but not work
    MyAbstr b=new A();//(Expected: compile) Actual: Compilation error - "Wrong type for the assignment"
}

@Otaka Otaka closed this Nov 16, 2015
@Otaka Otaka reopened this Nov 16, 2015
@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 17, 2015

In reference to the no-ops:

This is properly fixed with 5 lines of code in one place. (see next comment)

results with krzys-h's test program:

   test.testPrint(); // still works
   test.x = 2; // change base
   test.y = 1; // change child
   test.testPrint(); // still works
   test.testNoOverride("This should work"); //output: "This should work"   fixed.
   test.testInsideCallNoOverride(); //output: "testInsideCallNoOverride"   fixed.
   test.someOtherMethod(); //output: "this is TestClass::someOtherMethod"  still works
   test.testRecursion(5); //output: "Recursion works!"               still works
   test.testNoSuperCall(); //output "child" CBot checks child first  still works
   message("Finished without crashing!");//<--this too

It is also possible attach a public function to a class with "public void className::FuncName(){}"

public class ClassA
{    }

public void ClassA::DoStuff() // can be overridden with  DoStuff() { }  inside the class
{  //must be public
    message("this works too"); //unless the class overrides it.
}

extern void object::Test()
{
    ClassA aClass();
    aClass.DoStuff(); // output: "this works too"
}

A few issues worth noting:

Changing the class definition line after compiling it already, such as
adding or removing extends something requires saving the game, exiting the application entirely, and restarting the game, for the change to take effect.
I think it has something to do with it being public/global and preventing duplicate definitions.

message(""+this); in a class shows "pointer to classname(x=1, ...) extends otherclass(z=4, ...)"

Child class cannot access grandparent members, which I believe is proper behavior.

It is possible to "extends point" with expected results.

It is possible to "extends object" with expected results. radar still outputs to it <---very useful IMO

It is possible to instantiate an array of classes with:

classname myclass[];
myclass[0] = new classname();

Radar outputs to an array of object but not to an array of "someClass extends object"

@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 17, 2015

This code fixes inheritance: it goes between lines 368 and 369 in CBotClass.cpp current dev branch.
It was probably forgotten or deleted by mistake.

    if ( ret >= 0 ) return ret;

    if ( m_pParent != nullptr )
    {
        ret = m_pParent->m_pCalls->DoCall(nIdent, name, pThis, ppParams, pResult, pStack, pToken);
        if ( ret >= 0 ) return ret;
        ret = m_pParent->m_pMethod->DoCall(nIdent, name, pThis, ppParams, pStack, pToken, m_pParent);
    }
@krzys-h
Copy link
Member

@krzys-h krzys-h commented Nov 17, 2015

@melex750 Thanks, I'll test this in a moment

@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 17, 2015

There are also keywords "protected" and "final". These are highlighted in the editor.
It seems they only apply to variables.
this is a valid statment:

protected static int a = 4;

final compiled once?? I can't figure out what I typed. It needs further testing.

@Otaka
Copy link
Author

@Otaka Otaka commented Nov 18, 2015

In Java "final" for variables - it is "const" in c/c++
final for classes - class that cannot be subclassed.
final for method - method that cannot be overwritten in child classes.

@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 19, 2015

A fix for this issue:
Changing the class definition line after compiling it already, such as
adding or removing extends something requires saving the game, exiting the application entirely, and restarting the game, for the change to take effect.

           // CBotFunction.cpp between lines 1641 and 1642
            if ( pOld != nullptr && pOld->m_pParent == nullptr )
            {
                pOld->m_pParent = CBotClass::Find( p->GetString() );
            }

// and at the end of line 1643 same file, all current line numbers dev-branch
 else if ( pOld != nullptr ) pOld->m_pParent = nullptr;
krzys-h added a commit that referenced this issue Nov 21, 2015
krzys-h added a commit that referenced this issue Nov 21, 2015
see #540
@krzys-h
Copy link
Member

@krzys-h krzys-h commented Nov 21, 2015

I've just commited your changes with a few style fixes. I also fixed a possible bug when the base class could not be found (I just copied the error handling code from CBotClass::Compile1)

@krzys-h
Copy link
Member

@krzys-h krzys-h commented Nov 21, 2015

message(""+this); in a class shows "pointer to classname(x=1, ...) extends otherclass(z=4, ...)"

Since when does CBot know how to cast class to string?!? I didn't know this even existed.


Child class cannot access grandparent members, which I believe is proper behavior.

They can, and they should be allowed to.

public class Grandparent
{
    protected int x;
}

public class Parent extends Grandparent
{
}

public class Child extends Parent
{
    public void Child()
    {
        this.x = 5;
        message(this.x);

        x = 3;
        message(x);

        // This syntax is terrible and counter-intuitive - super should work only for methods. Nevertheless, it works correctly.
        super.x = 7;
        message(super.x);

        // This doesn't work - and there is no reason it should :P
        //super.super.x = 8;
        //message(super.super.x);
    }
}

extern void object::TestGrandparent()
{
    Child c = new Child();
}

It is possible to "extends object" with expected results. radar still outputs to it <---very useful IMO

That may be useful, but it breaks all the casting rules I'm aware of, so for me it's really counter-intuitive. :P
In any normal language you are not allowed to do something like this:

ChildClass c = new BaseClass();

Found another case with inheritance that is not work(or may be I still did not find the way how it should be written) - Polymorphism.

Your code is right, the CBot engine is wrong


There are also keywords "protected" [...]

And there is "private" too.
I did some tests - "protected" works as expected, but "private" doesn't work correctly (it does exactly the same as "protected")

public class SomeBaseClass
{
    public int a_public;
    protected int a_protected;
    private int a_private;

    public void SomeBaseClass(int a)
    {
        this.a_public = a;
        this.a_protected = a;
        this.a_private = a;
    }
}

public class SomeClass extends SomeBaseClass
{
    public void SomeClass(int a)
    {
        super.SomeBaseClass(a);
        this.a_public = a+1;
        this.a_protected = a+2;
        this.a_private = a+3; // Shouldn't compile, but it does
        super.a_private = 5; // This syntax makes no sense, but apparently also works (I would rather block using super to access variables at all)
    }

    public void test()
    {
        message("1: "+this.a_public);
        message("2: "+this.a_protected);
        message("3: "+this.a_private); // Shouldn't compile, but it does
    }
}

extern void object::TestProtectionLevels()
{

    SomeClass c = new SomeClass(0);
    c.test();
    message(c.a_public);
    //message(c.a_protected); // This is correct - doesn't compile
    //message(c.a_private); // This is correct - doesn't compile

}

[...] and "final"

It is highlighted indeed, but not implemented in the engine. I'll remove the highlighting in a moment so it doesn't confuse anyone.

@tomaszkax86
Copy link
Contributor

@tomaszkax86 tomaszkax86 commented Nov 21, 2015

So what are you saying is, class inheritance almost works? I'm more than surprised. It's definitely something good, we'll have to document it.
We could implement proper behavior for final or const. Java's final is not exactly like const because final doesn't prevent you from changing object state. const in C++ makes instances immutable. If we want to introduce C++'s const to CBot, we'll have to introduce const methods. For Java's final we just have to make variables immutable, but not necessarily content of objects and arrays. We can use const keyword for either.

@melex750
Copy link
Contributor

@melex750 melex750 commented Nov 21, 2015

Thanks for the info. and excellent work. :)

I thought extending object was ok because it's just an empty class of null pointers similar to point.
However, It's probably a good example of what not to do.

@Otaka
Copy link
Author

@Otaka Otaka commented Nov 28, 2015

Tested the build with new changes. All works fine except:
BaseClass b=new ChildClass();
That prevents me from using many interesting OOP patterns.

I think we can close this ticket because now in general inheritance works, and create new bug with name "Child class cannot be casted to BaseClass reference"

@melex750
Copy link
Contributor

@melex750 melex750 commented Dec 4, 2015

For documentation purposes:
You can access parent class members implicitly without super keyword,
it is only needed for accessing overridden class members.

Remaining issues with inheritance:
If what I read about inheritance is correct,
**subClass.grandParentMethod();**should work.(currently does not work, easily fixed)
If so, the previous: fixed calling not overridden methods is wrong(my mistake),
it should probably repeat until the upper-most class, in a loop, or with recursion, and
stop when it finds a method.

I also learned that parentClass constructors should be called before subClass constructors,
which does not happen currently, They're not called at all with a new instance of SubClass,
and should therefore be called manually in your SubClass constructor.

About Polymorphism:
It is already implemented but partially broken.
currently this type of code compiles and runs in CBOT:
BaseClass bClass[];
bClass[0] = new SubClass(1, 4);
bClass[1] = new SiblingClass();

All instances are created correctly but references like the following do not work properly.
bClass[1].someMethod(); // <--sees only BaseClass
bClass[0].x; // <-------------sees only BaseClass
The source code which handles this is commented out with 1 or 2 small errors.(easily fixed)

The code which handles inline instance declaration BaseClass aClass = new SubClass();
is already implemented but broken with a typo.

I have sorted most of this out already, new issue #683 ,I will post my findings soon.
It needs to be tested by someone who knows the rules how it should work.(<-that's not me)

@clop1000
Copy link

@clop1000 clop1000 commented May 3, 2016

Guys did you say that everything is working now? And i can use class inheritance without troubles? What are limitations?

Can i use static functions?

I just tested inheritance works only for first case.

public class class_2 extends class_1 - everything works.
public class class_3 extends class_2 - this does not work! It says me "undefined function"!

@MrSimbax
Copy link
Member

@MrSimbax MrSimbax commented Sep 10, 2016

You've probably forgotten about the protected keyword. private does the same as protected, so the latter is currently useless (or the other way around).

@krzys-h
Copy link
Member

@krzys-h krzys-h commented Sep 24, 2016

Done in #834

@krzys-h krzys-h closed this Sep 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants