Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Respecting naming conventions & other possible changes  (Read 15802 times)

0 Members and 2 Guests are viewing this topic.

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« on: July 19, 2010, 04:29:42 pm »
PySFML follows the C++ SFML implementation's naming conventions instead of the ones defined for Python in the PEP 8 (see http://www.python.org/dev/peps/pep-0008/). While these naming conventions may produce easy-to-read C++ code, it decreases Python code's readability. If I want to use both PySFML and another library then my code will be an ugly mix of two naming conventions - and I shall choose one of them for my own code.

This little example looks more like Python-coded C++ to me than Python code:
Code: [Select]
# Include the PySFML extension
from PySFML import sf

# Create the main window
window = sf.RenderWindow(sf.VideoMode(800, 600), "PySFML test")

# Create a graphical string to display
text = sf.String("Hello SFML")

# Start the game loop
running = True
while running:
    event = sf.Event()
    while window.GetEvent(event):
        if event.Type == sf.Event.Closed:
            running = False

    # Clear screen, draw the text, and update the window
    window.Clear()
    window.Draw(text)
    window.Display()

Whereas something like that would be easier to read and more "Python-oriented":
Code: [Select]
# Include the PySFML extension
from pysfml import sf

# Create the main window
window = sf.RenderWindow(sf.VideoMode(800, 600), "PySFML test")

# Create a graphical string to display
text = sf.String("Hello SFML")

# Start the game loop
running = True
while running:
    event = sf.Event()
    while window.get_event(event):
        if event.type == "closed":
            running = False

    # Clear screen, draw the text, and update the window
    window.clear()
    window.draw(text)
    window.display()

Yet PySFML 1.x has already been used in several projects, and it would would break compatibility to change the naming conventions now. But you should consider this for the PySFML 2 release. I could help if required.

So, what is your opinion on this subject?

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Respecting naming conventions & other possible changes
« Reply #1 on: July 19, 2010, 04:42:23 pm »
I totally agree, SFML bindings should always stick to the rules of their respective language.

I don't know if this is related to naming conventions, but you wrote this:
Code: [Select]
if event.type == "closed"
Is it really better (or more pythonic) to use strings here?
Laurent Gomila - SFML developer

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« Reply #2 on: July 19, 2010, 05:03:59 pm »
Quote from: "Laurent"
I don't know if this is related to naming conventions, but you wrote this:
Code: [Select]
if event.type == "closed"
Is it really better (or more pythonic) to use strings here?
Not really, it was just to give an example of how i would have done it, but it could also be written using constants, for which the convention is:
Quote
Constants are usually declared on a module level and written in all capital letters with underscores separating words.  Examples include MAX_OVERFLOW and TOTAL.

Tank

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1486
    • View Profile
    • Blog
    • Email
Respecting naming conventions & other possible changes
« Reply #3 on: July 20, 2010, 01:35:02 am »
Sounds good. I don't think this needs more discussion, since more common naming conventions for the target language are always more consistent with own code.

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« Reply #4 on: July 20, 2010, 03:18:23 am »
By the way i don't understand the choice of writing:
Code: [Select]
from PySFML import sf
when we could simply write:
Code: [Select]
import sfml
Then there are some other things we could change to be more python-friendly but which may require a PySFML-specific tutorial. I don't know if this would be better. If SFML was difficult to use, it would sure be better, but since SFML is already user-friendly in its C++ implementation...
For example, instead of this:
Code: [Select]
from PySFML import sf

image = sf.Image()

if image.LoadFromFile("tagada.png"):
    window = sf.RenderWindow(sf.VideoMode(800, 600), "PySFML test")
    sprite = sf.Sprite(image)

    while window.IsOpened():
        window.Clear()
        window.Draw(sprite)
        window.Display()

        event = sf.Event()
        while window.GetEvent(event):
            if event.Type == sf.Event.Closed:
                window.Close()

else:
    print "Unable to load tagada.png"

We could have something like that:
Code: [Select]
import sfml

window = sfml.RenderWindow(sfml.VideoMode(800, 600), "PySFML test")
sprite = sfml.Sprite(sfml.Image("tagada.png"))

while window.is_opened():
    window.clear()
    window.draw(sprite)
    window.display()

    for event in window.iterevents():
        if event.type == sfml.CLOSED:
            window.close()

Where sfml.Image.__init__ would raise a user-defined exception if it were unable to load the file.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Respecting naming conventions & other possible changes
« Reply #5 on: July 20, 2010, 08:18:30 am »
Looks good. This is exactly what I do in SFML.Net.
Laurent Gomila - SFML developer

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« Reply #6 on: July 20, 2010, 05:00:00 pm »
And what would you think of including a base class for event-based applications?
Code: [Select]
# inspired by ircbot

import sfml

STR_EVENTS = {sfml.CLOSED: "closed", sfml.RESIZED: "resized",
              sfml.LOST_FOCUS: "lost_focus", sfml.GAINED_FOCUS: "gained_focus",
              sfml.TEXT_ENTERED: "text_entered",
              sfml.KEY_PRESSED: "key_pressed", sfml.KEY_RELEASED: "key_released",
              #...
             }

class StopMainLoop(Exception): pass

class EventManager:
    def __init__(self, window):
        """Initializes self.window object."""
        self.window = window

    def process_queue(self):
        """Processes events in the queue calling self.on_X(event) where X is the event type."""
        for event in self.window.iterevents():
            if hasattr(self, "on_" + STR_EVENTS[event.type]):
                handler = getattr(self, "on_" + STR_EVENTS[event.type])
                handler(event)

    def prepare_frame(self):
        """Use this method to do some stuff before drawing."""
        pass

    def draw(self):
        """Use this method to draw the current frame on the self.window object."""
        pass

    def main_loop(self):
        """Calls self.prepare_frame(), self.draw() and self.process_queue() in a loop."""
        try:
            while True:
                self.prepare_frame()

                self.window.clear()
                self.draw()
                self.window.display()

                self.process_queue()
        except StopMainLoop:
            pass

    def stop(self):
        """Call this method to stop the main loop."""
        if self.window.is_opened():
            self.window.close()
        raise StopMainLoop

Users would not be forced to use it but it could help. An sample code would be:
Code: [Select]
import sfml

MOVE_PIXELS = {sfml.DOWN: (0,1), sfml.LEFT: (-1,0), sfml.UP: (0,1), sfml.RIGHT: (1,0)}

class MyApp(sfml.EventManager):
    def __init__(self, background, player):
        sfml.EventManager.__init__(self, sfml.RenderWindow())

        background_img = sfml.Image(background)
        vm = sfml.VideoMode(background_img.width(), background_img.height(), 32)
        self.window.create(vm, "MyApp")

        self.background = sfml.Sprite(background_img)
        self.player = sfml.Sprite(sfml.Image(background))

    def draw(self):
        self.window.draw(self.background)
        self.window.draw(self.player)

    def on_closed(self, _):
        self.stop()

    def on_key_pressed(self, event):
        if event.key.code == sfml.ESCAPE:
            self.stop()
        elif event.key.code in MOVE_PIXELS:
            self.player.move(*MOVE_PIXELS[event.key.code])

if __name__ == "__main__":
    MyApp("background.png", "player.png").main_loop()

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Respecting naming conventions & other possible changes
« Reply #7 on: July 20, 2010, 05:03:15 pm »
This is fine as an add-on, but not as part of the binding itself.
Laurent Gomila - SFML developer

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« Reply #8 on: July 20, 2010, 05:18:47 pm »
You are right, this is quite far away from the original C++ library.

Tank

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1486
    • View Profile
    • Blog
    • Email
Respecting naming conventions & other possible changes
« Reply #9 on: July 22, 2010, 01:51:14 am »
I don't agree with the loading thing upon construction time. My opinion is that exceptions should be avoided where possible and loading an image is such a case.

It's quite normal from the technical's perspective if an image fails to load (like with all other types of file operations where e.g. a file cannot be found, is locked or the user lacks proper permissions), so I think throwing an exception here could be misleading.

Also, LoadFromFile should be changed to throw an exception, too. Everything else would be inconsistent (why does loading in the construtor throw, but in another method not?).

If it's just for saving one line of code, I highly doubt this is a useful thing. ;)

dentuk

  • Newbie
  • *
  • Posts: 14
    • View Profile
Respecting naming conventions & other possible changes
« Reply #10 on: July 22, 2010, 02:44:07 am »
My idea was to handle errors (loading images was just an example) using exceptions rather than return values. The aim is not only to gain a few lines but also to prevent the code from being executed when a problem appears. This way you can handle errors at a higher level (the calling function instead of the called one) if you want to. It also helps debugging since you can explicit the problem in the exception's name and value. As I am used to working with exceptions I find it a lot more convenient, but you may disagree.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Respecting naming conventions & other possible changes
« Reply #11 on: July 22, 2010, 08:49:17 am »
In my opinion, if we implement constructors then the Load functions are not needed anymore. I think it's quite common in those languages to create a brand new object every time you want to load a new resource. This is, again, what I do in SFML.Net (and CSFML as well).
Heap allocation and copy/assignment have a high design and performance cost in C++, but not in "dynamic" languages.

I think we should look at how the standard library handles errors in Python. For example, what does loading an invalid file do? Raise an error or return False?
Laurent Gomila - SFML developer

Tank

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1486
    • View Profile
    • Blog
    • Email
Respecting naming conventions & other possible changes
« Reply #12 on: July 22, 2010, 10:45:50 am »
It raises an exception. :) (never liked that fact after all)

I find it unnatural to place all that stuff into the contructor. For example, when you throw in the loading into the constructor, you also have to put loading from memory and creating images into there.

The constructor is by design a method that initializes a class instance to get a consistent state. After construction (currently), the image is just empty and ready to be used, which I think is a good starting point.

When load and create methods are missing, this eliminates the possibility to reuse an object -- of course images are not really reused often (mostly they're created once upon loadtime), but I'm mainly talking about design here.

Heap allocation can be ignored, that won't be a problem. But it has still exactly the same cost (even a little more) in PySFML, since it's just a binding.

If we decide to provide suitable constructors, I'd definitely vote for keeping the create and load methods.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32498
    • View Profile
    • SFML's website
    • Email
Respecting naming conventions & other possible changes
« Reply #13 on: July 22, 2010, 11:31:08 am »
Quote
I find it unnatural to place all that stuff into the contructor. For example, when you throw in the loading into the constructor, you also have to put loading from memory and creating images into there.

Absolutely.

Quote
The constructor is by design a method that initializes a class instance to get a consistent state. After construction (currently), the image is just empty and ready to be used, which I think is a good starting point.

With loading constructors, the object will be in a consistent state as well.
I even think that having an empty valid object is not a good design. In C++ this is meaningful but not in languages where objects are pointers. Right now you can have:
- image == None
- image is valid but empty
- image is valid and loaded

It may not be clear what the difference between "None" and "empty" is. And I think there's none (an empty valid image brings nothing more than an invalid one). In the C binding this is the same: an image is a pointer, therefore when you have no image this is simply NULL, and when you have a valid image you create a new instance with the proper loading arguments.

Quote
When load and create methods are missing, this eliminates the possibility to reuse an object

Reusing an image doesn't mean anything, nothing in the instance is kept between two consecutive Load. Calling load really creates a whole new image every time.

Quote
Heap allocation can be ignored, that won't be a problem. But it has still exactly the same cost (even a little more) in PySFML, since it's just a binding.

I said it is a problem in C++ ;)
My point is that in C++, construction and initialization can sometimes happen in different places, this is the reason why we need default constructors and Load functions. In Python, they both can be done at the same time.

Quote
If we decide to provide suitable constructors, I'd definitely vote for keeping the create and load methods.

I still don't see the benefit of that
Code: [Select]
image = sf.Image("image1.png");
image.LoadFromFile("image2.png");

compared to that
Code: [Select]
image = sf.Image("image1.png");
image = sf.Image("image2.png");
Laurent Gomila - SFML developer

Tank

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1486
    • View Profile
    • Blog
    • Email
Respecting naming conventions & other possible changes
« Reply #14 on: July 22, 2010, 04:59:20 pm »
Quote from: "Laurent"
In the C binding this is the same: an image is a pointer, therefore when you have no image this is simply NULL, and when you have a valid image you create a new instance with the proper loading arguments.

I have to admit, that convinced me. Thanks for your detailed comment.

 

anything