SimulaVR / Simula

Linux VR Desktop
MIT License
2.97k stars 91 forks source link

The datatype `Rectangle` is not actually a rectangle #4

Closed georgewsinger closed 6 years ago

georgewsinger commented 7 years ago

The datatype Rectangle

data Rectangle = Rectangle {
  _rectangleSize :: V2 Int
  } deriving (Show, Eq, Ord)

is not actually a rectangle, as opposed to Rect:

data Rect a = Rect {
  _rectUpperLeft :: V2 a,
  _rectSize :: V2 a
  }

which is. This needs refactored.

Agnishom commented 7 years ago

How should this be handled?

Do we need the Rectangle type at all? Is it being used elsewhere in the code? I think it is good enough to remove that snippet altogether

krakrjak commented 7 years ago

Do it! Be the contributor you see in the world. Send a PR and I'll try a build.

georgewsinger commented 7 years ago

@Agnishom Welcome to the project. Any build you send over I'll also try.

Agnishom commented 7 years ago

I placed a pull request. I didn't try building it on my local machine, though.

georgewsinger commented 7 years ago

@Agnishom Your pull request leads to a build error. There are still some places in the code that use the (poorly named) Rectangle datatype: https://github.com/SimulaVR/Simula/search?utf8=%E2%9C%93&q=Rectangle&type=

Agnishom commented 7 years ago

@georgewsinger I fixed those. How about now?

georgewsinger commented 7 years ago

@Agnishom I now get the following build error on your pull request:

        Ambiguous occurrence ‘Rect’
        It could refer to either ‘Graphics.Rendering.OpenGL.Rect’,
                                 imported from ‘Graphics.Rendering.OpenGL’ at src/Simula/BaseCompositor/OpenGL.hs:13:1-32
                                 (and originally defined in ‘Graphics.Rendering.OpenGL.GL.Rectangles’)
                              or ‘Simula.BaseCompositor.Geometry.Rect’,
                                 imported from ‘Simula.BaseCompositor.Geometry’ at src/Simula/BaseCompositor/OpenGL.hs:16:1-37
                                 (and originally defined
                                    at src/Simula/BaseCompositor/Geometry.hs:(73,1)-(76,3))

You should be able to handle these errors yourself before submitting a pull request. Are you able to get the project to build? If you are having trouble, post another GitHub issue and I'll help you get through it.

krakrjak commented 7 years ago

I get the same build error that @georgewsinger is getting. After digging deeper, this is a bit of a rabbit hole. I tried simply hiding Rect from the OpenGL import list, but that's not good enough as the constructor for Rect has now changed to require the point as well as the size.

@Agnishom I think you are well on your way, but there are still a few details to work out.

lboklin commented 6 years ago

Issue made obsolete after move to Godot.