From 015e11d72fd62ff5a3d8ed208d52d3b9b0328a6f Mon Sep 17 00:00:00 2001 From: Taylor Bockman Date: Fri, 30 Aug 2019 13:41:05 -0700 Subject: [PATCH] Some serious lint rolling and updating points to have a color attribute --- clusterview/colors.py | 17 ++++++++ clusterview/mode_handlers.py | 32 ++++++++------- clusterview/opengl_widget.py | 63 +++++++++++++----------------- clusterview/point_manager.py | 10 ++++- clusterview/points.py | 93 +++++++++++++++++++++++--------------------- tests/test_point.py | 25 +++++++----- tests/test_point_manager.py | 28 ++++++------- tests/test_point_set.py | 58 ++++++++++++++------------- 8 files changed, 179 insertions(+), 147 deletions(-) create mode 100644 clusterview/colors.py diff --git a/clusterview/colors.py b/clusterview/colors.py new file mode 100644 index 0000000..216e27c --- /dev/null +++ b/clusterview/colors.py @@ -0,0 +1,17 @@ +from enum import Enum + + +class Color(str, Enum): + BLUE = 'BLUE' + BLACK = 'BLACK' + GREY = 'GREY' + + +# A simple map from Color -> RGBA 4-Tuple +# Note: The color values in the tuple are not RGB, but +# rather OpenGL percentage values for RGB. +COLOR_TO_RGBA = { + Color.GREY: (0.827, 0.827, 0.826, 0.0), + Color.BLUE: (0.118, 0.565, 1.0, 0.0), + Color.BLACK: (0.0, 0.0, 0.0, 0.0) +} diff --git a/clusterview/mode_handlers.py b/clusterview/mode_handlers.py index 7003af3..30fa28c 100644 --- a/clusterview/mode_handlers.py +++ b/clusterview/mode_handlers.py @@ -3,15 +3,16 @@ from enum import Enum from PyQt5.QtCore import QEvent, Qt from PyQt5.QtGui import QCursor +from .colors import Color from .exceptions import ExceededWindowBoundsError from .mode import Mode -from .opengl_widget import (get_bb_bottom_right, get_bb_top_left, - set_drawing_event, set_move_bb_top_left, +from .opengl_widget import (set_drawing_event, set_move_bb_top_left, set_move_bb_bottom_right, reset_move_bbs, viewport_height, viewport_width) from .points import PointSet from .point_manager import PointManager + class __ClickFlag: # This is the first stage. On mouse release it goes to @@ -31,6 +32,7 @@ class __ClickFlag: # to NONE - we are done. SELECTED_MOVED = 3 + # Size of point for drawing __POINT_SIZE = 8 @@ -69,6 +71,7 @@ def refresh_point_list(ctx): ctx.point_list_widget.update() + def __handle_add_point(ctx, event): """ Event handler for the add point mode. @@ -85,7 +88,7 @@ def __handle_add_point(ctx, event): __handle_info_updates(ctx, event) if (event.button() == Qt.LeftButton and - event.type() == QEvent.MouseButtonPress): + event.type() == QEvent.MouseButtonPress): # At this point we can be sure resize_gl has been called # at least once, so set the viewport properties of the @@ -97,8 +100,8 @@ def __handle_add_point(ctx, event): PointManager.point_set.clear_selection() try: - # No attribute at the moment. - PointManager.point_set.add_point(event.x(), event.y()) + # No attribute at the moment, default point color is Color.GREY. + PointManager.point_set.add_point(event.x(), event.y(), Color.GREY) except ExceededWindowBoundsError: # The user tried to place a point whos edges would be # on the outside of the window. We will just ignore it. @@ -111,6 +114,7 @@ def __handle_add_point(ctx, event): ctx.opengl_widget.update() ctx.point_list_widget.update() + def __handle_edit_point(ctx, event): # TODO: This function and delete definitely need to make sure they are # on a point we have. @@ -132,6 +136,7 @@ def __handle_edit_point(ctx, event): ctx.update() # after this remove the point from the list + def ogl_keypress_handler(ctx, event): """ A keypress handler attached to the OpenGL widget. @@ -159,13 +164,14 @@ def ogl_keypress_handler(ctx, event): elif ctx.mode is not Mode.OFF: ctx.mode = Mode.OFF - + # Also change the mouse back to normal ctx.opengl_widget.setCursor(QCursor(Qt.CursorShape.ArrowCursor)) ctx.status_bar.showMessage("") ctx.opengl_widget.update() + def __handle_move_points(ctx, event): """ A relatively complicated state machine that handles the process of @@ -185,14 +191,14 @@ def __handle_move_points(ctx, event): # If we release the mouse, we want to quickly alert drag mode. if (event.button() == Qt.LeftButton and - event.type() == QEvent.MouseButtonRelease): + event.type() == QEvent.MouseButtonRelease): __left_mouse_down = False # This if statement block is used to set the bounding box for # drawing and call the selection procedure. if (event.button() == Qt.LeftButton and - event.type() == QEvent.MouseButtonPress): + event.type() == QEvent.MouseButtonPress): __left_mouse_down = True @@ -210,10 +216,10 @@ def __handle_move_points(ctx, event): # Post-selection handlers if (__left_click_flag is __ClickFlag.SELECTION_BOX - and event.type() == QEvent.MouseMove): + and event.type() == QEvent.MouseMove): set_move_bb_bottom_right(event.x(), event.y()) - + elif (__left_click_flag is __ClickFlag.SELECTION_MOVE and __last_mouse_pos is not None and __left_mouse_down @@ -239,7 +245,6 @@ def __handle_move_points(ctx, event): if event.y() > __last_mouse_pos[1]: p.move(0, dy) - except ExceededWindowBoundsError: # This point has indicated a move would exceed # it's bounds, so we'll just go to the next @@ -260,12 +265,13 @@ def __handle_move_points(ctx, event): ctx.opengl_widget.update() + def __handle_delete_point(ctx, event): __handle_info_updates(ctx, event) if (event.button() == Qt.LeftButton and - event.type() == QEvent.MouseButtonPress): + event.type() == QEvent.MouseButtonPress): set_drawing_event(event) @@ -276,6 +282,7 @@ def __handle_delete_point(ctx, event): ctx.opengl_widget.update() ctx.point_list_widget.update() + def __handle_info_updates(ctx, event): """ Updates data under the "information" header. @@ -287,7 +294,6 @@ def __handle_info_updates(ctx, event): ctx.mouse_position_label.setText(f"{event.x(), event.y()}") - # Simple dispatcher to make it easy to dispatch the right mode # function when the OpenGL window is acted on. MODE_HANDLER_MAP = { diff --git a/clusterview/opengl_widget.py b/clusterview/opengl_widget.py index f6c3f15..d558a18 100644 --- a/clusterview/opengl_widget.py +++ b/clusterview/opengl_widget.py @@ -19,24 +19,11 @@ from OpenGL.GL import (glBegin, glClearColor, glColor3f, glEnable, glEnd, GL_LINE_LOOP, GL_POINTS, glPointSize, glVertex3f, glViewport) +from .colors import Color, COLOR_TO_RGBA from .exceptions import handle_exceptions, InvalidModeError, InvalidStateError from .mode import Mode from .point_manager import PointManager -class Color(Enum): - BLUE = 0 - BLACK = 1 - GREY = 2 - -# A simple map from Color -> RGBA 4-Tuple -# Note: The color values in the tuple are not RGB, but -# rather OpenGL percentage values for RGB. -COLOR_TO_RGBA = { - Color.GREY: (0.827, 0.827, 0.826, 0.0), - Color.BLUE: (0.118, 0.565, 1.0, 0.0), - Color.BLACK: (0.0, 0.0, 0.0, 0.0) -} - # Constants set based on the size of the window. __BOTTOM_LEFT = (0, 0) __WIDTH = None @@ -161,28 +148,28 @@ def paint_gl(): on the current mode to determine what action to perform on the current event. """ - if(__current_context.mode is Mode.OFF and + if(__current_context.mode is Mode.OFF and not PointManager.point_set.empty()): - - # We want to redraw on any change to Mode.OFF so points are preserved - - # without this, any switch to Mode.OFF will cause a blank screen to - # render. - draw_points(PointManager.point_set, Color.GREY) - - if (__current_context.mode in [Mode.ADD, Mode.EDIT, + + # We want to redraw on any change to Mode.OFF so points are preserved - + # without this, any switch to Mode.OFF will cause a blank screen to + # render. + draw_points(PointManager.point_set) + + if (__current_context.mode in [Mode.ADD, Mode.EDIT, Mode.MOVE, Mode.DELETE] and - __current_event is None and PointManager.point_set.empty()): + __current_event is None and PointManager.point_set.empty()): return if (__current_context.mode in [Mode.ADD, Mode.EDIT, Mode.DELETE] and - PointManager.point_set.empty()): + PointManager.point_set.empty()): return if (__current_context.mode is Mode.ADD or - __current_context.mode is Mode.DELETE or - __current_context.mode is Mode.LOADED): + __current_context.mode is Mode.DELETE or + __current_context.mode is Mode.LOADED): - draw_points(PointManager.point_set, Color.GREY) + draw_points(PointManager.point_set) elif __current_context.mode is Mode.EDIT: raise NotImplementedError("Drawing for EDIT not implemented.") @@ -191,17 +178,18 @@ def paint_gl(): # We have to repeatedly draw the points while we are showing the # move box. if not PointManager.point_set.empty(): - draw_points(PointManager.point_set, Color.GREY) + draw_points(PointManager.point_set) draw_selection_box(Color.BLACK) if (__move_bb_top_left is not None and - __move_bb_bottom_right is not None): + __move_bb_bottom_right is not None): # Mark points that are selected in the bounding box # and draw them using the normal function highlight_selection() - draw_points(PointManager.point_set, Color.GREY) + draw_points(PointManager.point_set) + def __clamp_x(x): """ @@ -214,6 +202,7 @@ def __clamp_x(x): x_w = (x / (__WIDTH / 2.0) - 1.0) return x_w + def __clamp_y(y): """ Y-coordinate clamping function that goes from mouse coordinates to @@ -225,6 +214,7 @@ def __clamp_y(y): y_w = -1.0 * (y / (__HEIGHT / 2.0) - 1.0) return y_w + def box_hit(tx, ty, x1, y1, x2, y2): """ Calculates whether or not a given point collides with the given bounding @@ -267,6 +257,7 @@ def box_hit(tx, ty, x1, y1, x2, y2): ty >= y1 and ty <= y2) + def highlight_selection(): """ Given the current move bounding box, highlights any points inside it. @@ -283,6 +274,7 @@ def highlight_selection(): else: point.unselect() + def draw_selection_box(color): """ When the move bounding box state is populated and the mode is set @@ -315,7 +307,6 @@ def draw_selection_box(color): # same y as the bottom right. bottom_left_corner = (__move_bb_top_left[0], __move_bb_bottom_right[1]) - glBegin(GL_LINE_LOOP) glColor3f(ct[0], ct[1], ct[2]) @@ -337,6 +328,7 @@ def draw_selection_box(color): glEnd() + def clear_selection(): """ A helper designed to be called from the main window @@ -347,7 +339,8 @@ def clear_selection(): if not PointManager.point_set.empty(): PointManager.point_set.clear_selection() -def draw_points(point_set, color): + +def draw_points(point_set): """ Simple point drawing function. @@ -364,10 +357,6 @@ def draw_points(point_set, color): raise InvalidStateError("Drawing context must be set before setting " + "drawing mode") - if not isinstance(color, Color): - raise ValueError("Color must exist in the Color enumeration") - - glViewport(0, 0, __WIDTH, __HEIGHT) glPointSize(PointManager.point_set.point_size) @@ -378,7 +367,7 @@ def draw_points(point_set, color): blue = COLOR_TO_RGBA[Color.BLUE] glColor3f(blue[0], blue[1], blue[2]) else: - ct = COLOR_TO_RGBA[color] + ct = COLOR_TO_RGBA[point.color] glColor3f(ct[0], ct[1], ct[2]) glVertex3f(__clamp_x(point.x), diff --git a/clusterview/point_manager.py b/clusterview/point_manager.py index 1ae1e21..30320a8 100644 --- a/clusterview/point_manager.py +++ b/clusterview/point_manager.py @@ -1,7 +1,9 @@ import json +from .colors import Color from .points import PointSet + class PointManager(): """ A state class that represents the absolute state of the @@ -26,7 +28,10 @@ class PointManager(): data['viewport_height']) for point in data['points']: - PointManager.point_set.add_point(point['x'], point['y']) + # We will need to cast the string representation of color + # back into a Color enum. + PointManager.point_set.add_point(point['x'], point['y'], + Color(point['color'])) @staticmethod def save(location): @@ -45,7 +50,8 @@ class PointManager(): for p in PointManager.point_set.points: data['points'].append({ 'x': p.x, - 'y': p.y + 'y': p.y, + 'color': p.color }) with open(location, 'w') as out_file: diff --git a/clusterview/points.py b/clusterview/points.py index 88b0de5..f941b60 100644 --- a/clusterview/points.py +++ b/clusterview/points.py @@ -1,7 +1,9 @@ from math import floor +from .colors import Color from .exceptions import ExceededWindowBoundsError + class Point: """ A class representing a point. A point @@ -9,23 +11,33 @@ class Point: it. """ - def __init__(self, x, y, point_size, viewport_width, viewport_height): + def __init__(self, x, y, color, point_size, + viewport_width, viewport_height): """ - Initializes a new point with a point_size bounding box. + Initializes a new point with a point_size bounding box, viewport + awareness, and a color. Initialized with additional viewport data to make sure the move function refuses to move a point outside the screen. @param x The x-coordinate. @param y The y-coordinate. + @param color The color of the point. @param point_size The size of the point in pixels. @param viewport_width The width of the viewport. @param viewport_height The height of the viewport. """ + + if not isinstance(color, Color): + raise ValueError("Point must be initialized with a color of " + + "type Color.") + self.__point_size = point_size self.__x = x self.__y = y + self.__color = color + self.__viewport_width = viewport_width self.__viewport_height = viewport_height @@ -54,6 +66,17 @@ class Point: return self.__selected @property + def color(self): + return self.__color + + @color.setter + def color(self, color): + if not isinstance(color, Color): + raise ValueError("Point color must be of type Color.") + + self.__color = color + + @property def attributes(self): return self.__attributes @@ -89,9 +112,9 @@ class Point: # the (x, y) for a point is the center of the square and we # do not want the EDGES to exceed the viewport bounds. if (x > self.__viewport_width - half_point or - y > self.__viewport_height - half_point or - x < half_point or - y < half_point): + y > self.__viewport_height - half_point or + x < half_point or + y < half_point): raise ExceededWindowBoundsError @@ -120,6 +143,8 @@ class Point: """ return (self.__x == other.x and self.__y == other.y and + self.__color == other.color and + self.__attributes == other.attributes and self.__point_size == other.point_size) def __repr__(self): @@ -127,10 +152,11 @@ class Point: # For some reason I had to split this instead of using one giant # string chained with `+` inside of `()`. s = "= self.__bottom_right_corner[1]) + class Attribute: def __init__(self, name, value): @@ -183,6 +210,7 @@ class Attribute: self.__name = name self.__value = value + class PointSet: """ Useful container for points. Since points are not hashable (they are @@ -207,47 +235,20 @@ class PointSet: self.__viewport_height = viewport_height def __eq__(self, other): - # We are forced to convert other.points from generator to a set to - # check equality on the sets. This could possibly get very slow - # for large numbers of points. - - attributes_equal = True - other_points = list(other.points) - attributes_equal = (attributes_equal and - (len(self.__points) == len(other_points))) - - # This is O(N^2) - can it be improved using some sort of - # find function? - for p in self.__points: - for i, op in enumerate(other_points): - if p == op: - attributes_equal = (attributes_equal and - (p.attributes == op.attributes)) - continue - - if i == len(other_points) - 1: - # In this case we have enumerated the entire second - # set and not found anything. We can safely say - # the two sets are not equal and return. - attribute_equals = False - break - - return (self.__points == other_points and - attributes_equal and self.__point_size == other.point_size and self.__viewport_width == other.viewport_width and self.__viewport_height == other.viewport_height) def __repr__(self): - s = [] + s = [] - for p in self.__points: - s.append(str(p)) + for p in self.__points: + s.append(str(p)) - return ",".join(s) + return ",".join(s) @property def points(self): @@ -288,24 +289,28 @@ class PointSet: for p in self.__points: p.unselect() - def add_point(self, x, y, attrs=[]): + def add_point(self, x, y, color, attrs=[]): """ Adds a point in screen coordinates and an optional attribute to the list. @param x The x-coordinate. @param y The y-coordinate. + @param color The color of the point. @param attr An optional attribute. @raises ExceededWindowBoundsError If the point could not be constructed - because it would be outside the window - bounds. + because it would be outside the + window bounds. """ if attrs != [] and not all(isinstance(x, Attribute) for x in attrs): raise ValueError("Attributes in add_point must be an " + "attribute array.") - point = Point(x, y, self.__point_size, + if not isinstance(color, Color): + raise ValueError("Point color must be a Color enum.") + + point = Point(x, y, color, self.__point_size, self.__viewport_width, self.__viewport_height) for attr in attrs: diff --git a/tests/test_point.py b/tests/test_point.py index 0e427cb..1c39a7f 100644 --- a/tests/test_point.py +++ b/tests/test_point.py @@ -1,42 +1,49 @@ import pytest +from clusterview.colors import Color from clusterview.exceptions import ExceededWindowBoundsError from clusterview.points import Point, PointSet + def test_move_point(): # The minimum starting position is 1/2 point away # from the edges - p = Point(4, 4, 8, 100, 100) + p = Point(4, 4, Color.GREY, 8, 100, 100) p.move(1, 1) assert p.x == 5 and p.y == 5 + def test_attributes_must_be_array_of_attributes(): with pytest.raises(ValueError): - l = PointSet(8, 100, 100) - l.add_point(4, 4, attrs=[1,2,3,4,5]) + point_set = PointSet(8, 100, 100) + point_set.add_point(4, 4, Color.GREY, attrs=[1, 2, 3, 4, 5]) + def test_move_point_outside_screen_x_positive(): - p = Point(4, 4, 8, 100, 100) + pt = Point(4, 4, Color.GREY, 8, 100, 100) + + with pytest.raises(ExceededWindowBoundsError): + pt.move(96, 0) - with pytest.raises(ExceededWindowBoundsError) as exc_info: - p.move(96, 0) def test_move_point_outside_screen_y_positive(): - p = Point(4, 4, 8, 100, 100) + p = Point(4, 4, Color.GREY, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): p.move(0, 95) + def test_move_point_outside_screen_x_negative(): - p = Point(4, 4, 8, 100, 100) + p = Point(4, 4, Color.GREY, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): p.move(-5, 0) + def test_move_point_outside_screen_y_negative(): - p = Point(4, 4, 8, 100, 100) + p = Point(4, 4, Color.GREY, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): p.move(0, -5) diff --git a/tests/test_point_manager.py b/tests/test_point_manager.py index 42b980e..fc14648 100644 --- a/tests/test_point_manager.py +++ b/tests/test_point_manager.py @@ -3,6 +3,7 @@ import os import pytest +from clusterview.colors import Color from clusterview.points import PointSet from clusterview.point_manager import PointManager @@ -10,20 +11,21 @@ from clusterview.point_manager import PointManager @pytest.fixture(autouse=True) def setup(): p = PointSet(8, 100, 100) - p.add_point(4, 4) - p.add_point(9, 10) - p.add_point(30, 40) + p.add_point(4, 4, Color.GREY) + p.add_point(9, 10, Color.GREY) + p.add_point(30, 40, Color.GREY) PointManager.point_set = p + def test_load(tmpdir): test = ("{\n" "\"point_size\": 8,\n" "\"viewport_height\": 100,\n" "\"viewport_width\": 100,\n" "\"points\":[\n" - "{\"x\": 8, \"y\": 8}," - "{\"x\": 30, \"y\": 50}" + "{\"x\": 8, \"y\": 8, \"color\": \"GREY\"}," + "{\"x\": 30, \"y\": 50, \"color\": \"GREY\"}" "]\n" "}") @@ -31,17 +33,16 @@ def test_load(tmpdir): p.write(test) expected = PointSet(8, 100, 100) - expected.add_point(8, 8) - expected.add_point(30, 50) + expected.add_point(8, 8, Color.GREY) + expected.add_point(30, 50, Color.GREY) PointManager.load(p) - print(PointManager.point_set) - # The fixture point_set inside the singleton PointManager should be # overwritten. assert PointManager.point_set == expected + def test_save(tmpdir): d = tmpdir.mkdir("test_data").join("save.json") @@ -52,16 +53,15 @@ def test_save(tmpdir): "\"viewport_height\": 100," "\"viewport_width\": 100," "\"points\":[" - "{\"x\": 4, \"y\": 4}," - "{\"x\": 9, \"y\": 10}," - "{\"x\": 30, \"y\": 40}" + "{\"x\": 4, \"y\": 4, \"color\": \"GREY\"}," + "{\"x\": 9, \"y\": 10, \"color\": \"GREY\"}," + "{\"x\": 30, \"y\": 40, \"color\": \"GREY\"}" "]" "}") - with open(d) as f: expected = json.loads(expected_str) - actual = json.load(d) + actual = json.load(f) # Since the JSON module converts the `points` key to # a list of dicts, we need to do manual comparison diff --git a/tests/test_point_set.py b/tests/test_point_set.py index 53e625b..95c0efe 100644 --- a/tests/test_point_set.py +++ b/tests/test_point_set.py @@ -1,89 +1,91 @@ import pytest +from clusterview.colors import Color from clusterview.points import Attribute, Point, PointSet def test_empty(): - l = PointSet(3, 100, 100) + point_set = PointSet(3, 100, 100) + + assert point_set.empty() - assert l.empty() def test_add_to_point_set(): - l = PointSet(3, 100, 100) + point_set = PointSet(3, 100, 100) - l.add_point(1, 2) + point_set.add_point(1, 2, Color.GREY) - points = list(l.points) + points = list(point_set.points) - p = Point(1, 2, 3, 100, 100) + p = Point(1, 2, Color.GREY, 3, 100, 100) assert len(points) == 1 assert points[0] == p + def test_add_to_point_set_with_attributes(): attribute = Attribute("thing", 1) - l = PointSet(3, 100, 100) - l.add_point(2, 3, attrs=[attribute]) + point_set = PointSet(3, 100, 100) + point_set.add_point(2, 3, Color.GREY, attrs=[attribute]) - points = list(l.points) - point = Point(2, 3, 3, 100, 100) + points = list(point_set.points) assert len(points) == 1 assert len(points[0].attributes) == 1 + def test_remove_point_exact_click(): attribute = Attribute("thing", 1) - l = PointSet(8, 100, 100) - l.add_point(4, 4, attrs=[attribute]) + point_set = PointSet(8, 100, 100) + point_set.add_point(4, 4, Color.GREY, attrs=[attribute]) - p = Point(4, 4, 8, 100, 100) - l.remove_point(4, 4) + point_set.remove_point(4, 4) - points = list(l.points) + points = list(point_set.points) assert len(points) == 0 + def test_remove_point_bounding_box(): """ This test checks the bounding box hit heuristic. """ attribute = Attribute("thing", 1) - l = PointSet(8, 100, 100) - l.add_point(4, 4, attrs=[attribute]) - - p = Point(4, 4, 8, 100, 100) + point_set = PointSet(8, 100, 100) + point_set.add_point(4, 4, Color.GREY, attrs=[attribute]) # The click-point (2, 1) will be inside of our point size 8 # bounding box. - l.remove_point(4, 4) + point_set.remove_point(4, 4) - points = list(l.points) + points = list(point_set.points) assert len(points) == 0 + def test_clear_all_selected_points(): - l = PointSet(8, 100, 100) - l.add_point(4, 4) - l.add_point(5, 5) + point_set = PointSet(8, 100, 100) + point_set.add_point(4, 4, Color.GREY) + point_set.add_point(5, 5, Color.GREY) - for p in l.points: + for p in point_set.points: p.select() selected = 0 - for p in l.points: + for p in point_set.points: if p.selected: selected += 1 assert selected == 2 - l.clear_selection() + point_set.clear_selection() unselected = 0 - for p in l.points: + for p in point_set.points: if not p.selected: unselected += 1