Visualization area update 1 #47

Artem-Sotnikov · 2020-07-12T01:04:29Z

The first batch of features to be added to the visualization area including:

Context menu
On/Off buttons for visual elements
Zooming, panning, resetting
Sprite loader
Grid
Graphical representations of components
General graphical improvements

This change is

Code is a working version that is missing comments and formatting. However, several new UI features are functional and the groundwork for adding more functionality is complete.

All methods and classeshave been cleaned and commented. Unecessary files were removed. Moreover, several features have been added: - action freeze upon context menu opening - selective context menu requests - parrot mode demo

wendi-yu

Looks great, I love the new functionality being added! A few small general UX comments:

the change colours button doesn't seem to be able to change back
same with parrot mode

Overall I really like where our frontend is going (also debug mode seems very useful, glad you put it in :) )

Reviewable status: 0 of 14 files reviewed, 11 unresolved discussions (waiting on @CODE-TITAN, @jacobdeery, and @wendi-yu)

application/resources/PlumbingPane.qml, line 31 at r1 (raw file):

        Connections {
            target: visualization_area

whitespace :0

application/resources/PlumbingPane.qml, line 140 at r1 (raw file):

                text_field1.text = "Node identifier: " + node_data
                text_field2.text = "Node identifier: " + comp_data
            }

whitespace :0

application/resources/PlumbingPane.qml, line 232 at r1 (raw file):

            height: parent.height
            width: parent.width

extra whitespace here :)

application/vis_area_res/graphics_component.py, line 24 at r1 (raw file):

            print('load sucessful!')

    def calculate_bouding_rect(self):

bounding?

application/vis_area_res/visualization_area.py, line 35 at r1 (raw file):

               'c12': {1: 6, 2: 'atm'}}

    pressures = {}

It's fine for pressures to be left empty if you don't need values for any of the nodes, the default value is (0, False).

application/vis_area_res/visualization_area.py, line 120 at r1 (raw file):

        # Instantiates the graphics components based on the dict in engine instance
        self.graphics_components = {key: GraphicsComponent(
            key, 'valve') for key in self.engine_instance.component_dict.keys()}

This is more a plumbing engine problem than a visualization one, but it would probably be better to use an access method that yields the components here than to be reaching into the bowels of the engine directly - using an API is generally less fragile. I'll add the method, at which point we can change this?

application/vis_area_res/visualization_area.py, line 135 at r1 (raw file):

        for comp_key in self.graphics_components.keys():
            comp = self.graphics_components[comp_key]
            comp.calculate_bouding_rect()

bounding again

application/vis_area_res/visualization_area.py, line 144 at r1 (raw file):

        This is a temporary function that quickly adapts the output of the test function such
        that it could work with the display using hard-coded adjustment values.

Extra space at the end of this line (nit)

application/vis_area_res/visualization_area.py, line 216 at r1 (raw file):

        # Draws grid if necessary
        if self.grid_visible:
            for draw_idx in range(0, 10):

nit but I'm pretty sure in range(10) is fine, 0 is the default start value. At some point it might also be a nice UX detail to have the number of grid lines scale with the zoom level, though what we have for now is great.

application/vis_area_res/visualization_area.py, line 251 at r1 (raw file):

            for edge in t.edges:

                # For correct calculations later on, precedence  of points must be evaluated

Extra space between "precedence" and "of" (nit)

application/vis_area_res/visualization_area.py, line 323 at r1 (raw file):

        Parameters
        ----------

Do we use - or = for the underline? Not a huge deal in this PR, but we might want to go through and choose a consistent style at some point in time. Maybe in the same pass as the one to standardize naming style.

application/vis_area_res/visualization_area.py, line 411 at r1 (raw file):

            # While the context menu is open, no further actions should be registered.
            # The qml will issue a call `unfreeze_display()` when it is done

qml -> QML

application/vis_area_res/visualization_area.py, line 474 at r1 (raw file):

        Handle the wheel event for a movement of the scroll wheel on the paint surface.

        Is called automatically by the object whenever the scroll wheel is moved.

extra whitespace at the end of the line

application/vis_area_res/visualization_area.py, line 572 at r1 (raw file):

        Force a re-paint to occur on the draw surface.

        This method is accessible to QML, so QML objects can use it to froce repaints

froce -> force

wendi-yu

Reviewable status: 0 of 14 files reviewed, 11 unresolved discussions (waiting on @CODE-TITAN and @jacobdeery)

application/vis_area_res/visualization_area.py, line 216 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

nit but I'm pretty sure in range(10) is fine, 0 is the default start value. At some point it might also be a nice UX detail to have the number of grid lines scale with the zoom level, though what we have for now is great.

Also for the sake of consistency, might be better to have in range(num_vertical_lines). If this will always be the same as num_horizontal_lines it could be helpful to combine them too, since changing multiple hard coded values later on is a pain

application/vis_area_res/visualization_area.py, line 277 at r1 (raw file):

                    print('edge end 2: ' + str(p2[0]) + str(p2[1]))

                # Line is actually drawn with clipping considered

I like this effect, it feels very polished :)

jacobdeery

I really like the look of this! There's a bit of work to get everything into tiptop shape, but I think this is a great next step for the visualization.

General comments:

The format script seems to be breaking trying to format a deleted file. I'll fix that and put up a PR; after that gets merged, can you run tools/format.sh to make sure everything is good?
This PR requires upgrading PySide2 and shiboken2 to 5.14.1 - please update this in requirements.txt.

Reviewable status: 0 of 14 files reviewed, 51 unresolved discussions (waiting on @CODE-TITAN and @jacobdeery)

.gitignore, line 18 at r1 (raw file):

Quoted 7 lines of code…

# Qt Designer Config Files
topside_qtcreator.cflags
topside_qtcreator.config
topside_qtcreator.creator
topside_qtcreator.cxxflags
topside_qtcreator.files
topside_qtcreator.includes

Will we ever need to track any topside_qtcreator files at all, or can we just gitignore topside_qtcreator.*?

application/application.py, line 8 at r1 (raw file):

.vis_area_res

I think plumbing_vis is a better name for this directory. It's not a "resources" directory, since it only contains code.

application/resources/PlumbingPane.qml, line 1 at r1 (raw file):

import QtQuick 2.0

Lots of extra newlines and extraneous whitespace throughout this file

application/resources/PlumbingPane.qml, line 32 at r1 (raw file):

onRequest_QML_context

Is this the name that automatically gets generated if we register a signal named request_QML_context? If so, that's enough reason to use camelCase for naming our signals and slots, in my opinion.

application/resources/PlumbingPane.qml, line 200 at r1 (raw file):

(!isChecked)

Nit: no need for parens

application/resources/component_icons/parrot.jpg, line 0 at r1 (raw file):
What's the licensing on this image?

application/resources/component_icons/valve_icon.png, line 0 at r1 (raw file):
Same comment

application/resources/gui_icons/components_visible_icon.png, line 0 at r1 (raw file):
You said that you made these ones yourself, right?

application/vis_area_res/graphics_component.py, line 8 at r1 (raw file):

    """
    A object that acts as the graphical representation of a component.

Nit: remove extra newline

application/vis_area_res/graphics_component.py, line 11 at r1 (raw file):

component_id

The term "component ID" refers to the unique name for a given component; I think this should be component_type here.

application/vis_area_res/graphics_component.py, line 18 at r1 (raw file):

'application/resources/component_icons/' + component_id + '_icon.png'

This relative filepath load will fail if this function is called from anywhere other than the root topside directory, and will also fail on the standalone application since the folder structure is different.

The solution is to use the find_resource function currently defined in application.py - right now, however, that won't work, because it will introduce a circular dependency. We'll need to move find_resource to a new utils file where application.py and this file can safely include it without worries. I can do that as a separate PR if you're not sure what needs to be done, but if it makes sense to you, go ahead and include it with this one.

application/vis_area_res/graphics_component.py, line 20 at r1 (raw file):

print('could not load \'' + component_id + '\'!')

When a string contains single or double quotes, the string itself should be quoted with the other form so that backslash escapes aren't needed. Consider using this form, which is a bit clearer:

print(f'could not load "{component_id}"!')

Consider also adding the filepath that failed to load, so that it's easier to debug.

application/vis_area_res/graphics_component.py, line 27 at r1 (raw file):

        """
        Calculate the bounding rectangle of the component based on what nodes it owns.

remove newline

application/vis_area_res/graphics_component.py, line 45 at r1 (raw file):

Where does this padding come from? Is this chosen arbitrarily? Should it always be the same for x and y?

application/vis_area_res/graphics_component.py, line 60 at r1 (raw file):

        painter: QPainter
            The given painter which will execute the graphical commands given to it.

remove newline

application/vis_area_res/graphics_node.py, line 9 at r1 (raw file):

    Inherits PySide2.QtCore.QRectF.

remove newline

application/vis_area_res/visualization_area.py, line 8 at r1 (raw file):

from numpy import arctan, cos, sin, round, pi

Should be grouped with the PySide2 imports

Consider import numpy as np instead so that it's clear what namespace everything is coming from

application/vis_area_res/visualization_area.py, line 10 at r1 (raw file):

# Note: Copied from layout_demo to avoid using importlib

Please restore this comment to the form it's currently in on master

application/vis_area_res/visualization_area.py, line 56 at r1 (raw file):

NODE_RAD = 5

NODE_RADIUS, RAD can be short for "radians" and it wasn't immediately clear to me what this name meant

application/vis_area_res/visualization_area.py, line 85 at r1 (raw file):

self.zoom_state = 1

maybe zoom_factor? state typically implies a discrete set rather than a continuous spectrum

application/vis_area_res/visualization_area.py, line 90 at r1 (raw file):

Quoted 4 lines of code…

        self.temporary_x_transpose_state = 0
        self.temporary_y_transpose_state = 0
        self.permanent_x_transpose_state = 0
        self.permanent_y_transpose_state = 0

What do these variables represent?

application/vis_area_res/visualization_area.py, line 105 at r1 (raw file):

        Any items that can be initialized without an active QPainter instance but will be used in
        the paint method should be initialized and calculated once in this method.

remove newline

application/vis_area_res/visualization_area.py, line 111 at r1 (raw file):

        t = self.terminal_graph
        pos = self.layout_pos

any reason for these local variables? they're only used once and never mutated

application/vis_area_res/visualization_area.py, line 119 at r1 (raw file):

key

use name so that it's clear what the keys actually are

application/vis_area_res/visualization_area.py, line 124 at r1 (raw file):

        for comp_key in self.graphics_components.keys():
            for g_node_key in self.graphics_nodes.keys():

same here

application/vis_area_res/visualization_area.py, line 129 at r1 (raw file):

(comp_key + '.') in str(g_node_key)

should this be str(g_node_key).startswith(comp_key + '.')? Otherwise if, for some reason, we have a node named valve1.valve2.node, it will be identified as belonging to valve2.

application/vis_area_res/visualization_area.py, line 145 at r1 (raw file):

        This is a temporary function that quickly adapts the output of the test function such
        that it could work with the display using hard-coded adjustment values.

remove newline

application/vis_area_res/visualization_area.py, line 169 at r1 (raw file):

            self.graphics_nodes[node] = GraphicsNode(pt, self.NODE_RAD, node)

    def paint(self, painter):

This is a very long function, and it's quite difficult to read as a result. Please consider splitting it up into several smaller functions performing discrete steps (setting up fonts/pens/brushes, drawing the grid, drawing nodes and edges, etc.)

application/vis_area_res/visualization_area.py, line 210 at r1 (raw file):

        num_horiz_lines = 10
        num_vertical_lines = 10

Extract as constants

application/vis_area_res/visualization_area.py, line 212 at r1 (raw file):

        vertical_interval = self.initial_bounds.height() / num_vertical_lines
        horiz_interval = self.initial_bounds.width() / num_horiz_lines

Swap these two lines so they're in the same order as the previous two (horizontal and then vertical)

application/vis_area_res/visualization_area.py, line 253 at r1 (raw file):

Quoted 13 lines of code…

                if pos[edge[0]][0] == pos[edge[1]][0]:
                    if pos[edge[0]][1] < pos[edge[1]][1]:
                        p1 = pos[edge[0]]
                        p2 = pos[edge[1]]
                    else:
                        p1 = pos[edge[1]]
                        p2 = pos[edge[0]]
                elif pos[edge[0]][0] < pos[edge[1]][0]:
                    p1 = pos[edge[0]]
                    p2 = pos[edge[1]]
                else:
                    p1 = pos[edge[1]]
                    p2 = pos[edge[0]]

With all of the indexing and similar names here, it's very difficult to understand what this logic is doing.

I have a few suggestions to make it easier to untangle:

Extract pos[edge[0]] and pos[edge[1]] into named variables beforehand so there's only ever one level of indexing going on ([0] and [1] for x and y)
Handle the nominal cases (x0 < x1 and x0 > x1) first, before the special case that is x0 == x1.
Make the comment more descriptive: right now it's not at all clear that we need to order the points because we want to clip the line unless you go and read ahead.

application/vis_area_res/visualization_area.py, line 267 at r1 (raw file):

(p2[0] - p1[0]) == 0:

p1[0] == p2[0]

application/vis_area_res/visualization_area.py, line 270 at r1 (raw file):

((p2[0] - p1[0]))

remove extra parens

application/vis_area_res/visualization_area.py, line 281 at r1 (raw file):

Quoted 4 lines of code…

                painter.drawLine(p1[0] + (self.NODE_RAD + 1)*cos(theta_edge),
                                 p1[1] + (self.NODE_RAD + 1)*sin(theta_edge),
                                 p2[0] - (self.NODE_RAD)*cos(theta_edge), p2[1]
                                 - (self.NODE_RAD)*sin(theta_edge))

Why only adding 1 for the first point?

application/vis_area_res/visualization_area.py, line 502 at r1 (raw file):

        event.accept()

    def upload_engine_instance(self, engine):

This function should probably be much further up in this class; right now it's in the middle of some QML functions, and it doesn't really fit here

application/vis_area_res/visualization_area.py, line 504 at r1 (raw file):

Sets

Set

application/vis_area_res/visualization_area.py, line 512 at r1 (raw file):

plumbing_engine

PlumbingEngine

application/vis_area_res/visualization_area.py, line 552 at r1 (raw file):

    @Slot(str)
    def log_from_qml(self, text):

I really like this idea - will be very useful once we add our own logging system.

application/vis_area_res/visualization_area.py, line 557 at r1 (raw file):

python

Python

application/vis_area_res/visualization_area.py, line 705 at r1 (raw file):

Demo sprite accessing

End with period

Also add copy protection for the errors accessor, to keep changes from propagating through to the engine's sets/dicts.

Some issues that have come up have been fixed.

Add a diff-filter to the git diff to exclude deleted files.

Code is a working version that is missing comments and formatting. However, several new UI features are functional and the groundwork for adding more functionality is complete.

All methods and classeshave been cleaned and commented. Unecessary files were removed. Moreover, several features have been added: - action freeze upon context menu opening - selective context menu requests - parrot mode demo

Some issues that have come up have been fixed.

…rloo-rocketry/topside into visualization_area_update_1

Artem-Sotnikov

Reviewable status: 0 of 17 files reviewed, 49 unresolved discussions (waiting on @CODE-TITAN, @jacobdeery, and @wendi-yu)

.gitignore, line 18 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

# Qt Designer Config Files
topside_qtcreator.cflags
topside_qtcreator.config
topside_qtcreator.creator
topside_qtcreator.cxxflags
topside_qtcreator.files
topside_qtcreator.includes
Will we ever need to track any topside_qtcreator files at all, or can we just gitignore topside_qtcreator.*?

I didn't know you could do that in gitignore, thanks!

application/application.py, line 8 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

.vis_area_res
I think plumbing_vis is a better name for this directory. It's not a "resources" directory, since it only contains code.

I agree.

application/resources/PlumbingPane.qml, line 31 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

whitespace :0

Done.

application/resources/PlumbingPane.qml, line 140 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

whitespace :0

Done.

application/resources/PlumbingPane.qml, line 200 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

(!isChecked)
Nit: no need for parens

Done.

application/resources/PlumbingPane.qml, line 232 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

extra whitespace here :)

Done.

application/resources/component_icons/parrot.jpg, line at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

What's the licensing on this image?

I've checked and it's a Getty Images image: "Photo: Francois Nel/Getty Images". Which means that we can't use it in any sort of distributed project. It's meant as a placeholder - as soon as we get a proper sprite sheet we can change it up.

application/resources/component_icons/valve_icon.png, line at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Same comment

That's a logo, so it will definitely have to go when we have a proper sprite sheet.

application/resources/gui_icons/components_visible_icon.png, line at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

You said that you made these ones yourself, right?

That's right

application/vis_area_res/graphics_component.py, line 8 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Nit: remove extra newline

Done.

application/vis_area_res/graphics_component.py, line 11 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

component_id
The term "component ID" refers to the unique name for a given component; I think this should be component_type here.

Done.

application/vis_area_res/graphics_component.py, line 20 at r1 (raw file):

print(f'could not load "{component_id}"!')
I didn't know you could do that in Python, thanks!

As for the debug, all of them currently search the same folder, so I'm not sure if it's useful to say which every time.
Nonetheless, I'll add a line that says which folder was searched.

application/vis_area_res/graphics_component.py, line 24 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

bounding?

Done.

application/vis_area_res/graphics_component.py, line 27 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

remove newline

Done.

application/vis_area_res/graphics_component.py, line 45 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

10
Where does this padding come from? Is this chosen arbitrarily? Should it always be the same for x and y?

I chose it somewhat arbitrarily. This way, the component edge will be at most a distance fo 10 from the center of any node it owns. It could be different for and x and y, but this would affect the boundary of the component (the rectangle that shows up for every component).

application/vis_area_res/graphics_node.py, line 9 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

remove newline

Done.

application/vis_area_res/visualization_area.py, line 8 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

from numpy import arctan, cos, sin, round, pi
Should be grouped with the PySide2 imports

Consider import numpy as np instead so that it's clear what namespace everything is coming from

For basic mathematical operators, does it really matter where they are being imported from? I chose numpy rather arbitrarily since I have some experience with it in the past.

application/vis_area_res/visualization_area.py, line 10 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

# Note: Copied from layout_demo to avoid using importlib
Please restore this comment to the form it's currently in on master

Huh, I don't know how that got reverted. Fixed.

application/vis_area_res/visualization_area.py, line 56 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

NODE_RAD = 5
NODE_RADIUS, RAD can be short for "radians" and it wasn't immediately clear to me what this name meant

Done.

application/vis_area_res/visualization_area.py, line 85 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

self.zoom_state = 1
maybe zoom_factor? state typically implies a discrete set rather than a continuous spectrum

That's a good point. Done.

application/vis_area_res/visualization_area.py, line 90 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        self.temporary_x_transpose_state = 0
        self.temporary_y_transpose_state = 0
        self.permanent_x_transpose_state = 0
        self.permanent_y_transpose_state = 0

What do these variables represent?

How much the window is panned by (in total for 'permanent', and during the current drag operation for 'temporary'). I'll add a comment describing this, and I'll also change it to 'factor' to fit with the sentiment of the past comment.

application/vis_area_res/visualization_area.py, line 105 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

remove newline

Done.

application/vis_area_res/visualization_area.py, line 111 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        t = self.terminal_graph
        pos = self.layout_pos
any reason for these local variables? they're only used once and never mutated

This is a carryover from me copy + pasting the draw method from tests. I'll change it so it accesses the terminal graph directly. However, layout_pos is used a lot in the later function so shortening it to a local variable pos is still useful so I'll keep that it.

application/vis_area_res/visualization_area.py, line 119 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

key
use name so that it's clear what the keys actually are

Done.

application/vis_area_res/visualization_area.py, line 120 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

This is more a plumbing engine problem than a visualization one, but it would probably be better to use an access method that yields the components here than to be reaching into the bowels of the engine directly - using an API is generally less fragile. I'll add the method, at which point we can change this?

Updated with the new method.

application/vis_area_res/visualization_area.py, line 124 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        for comp_key in self.graphics_components.keys():
            for g_node_key in self.graphics_nodes.keys():

same here

Since I'm still iterating through a dict at this point, I think 'key' is also descriptive. I'll change it to comp_name_key though.

application/vis_area_res/visualization_area.py, line 129 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

(comp_key + '.') in str(g_node_key)
should this be str(g_node_key).startswith(comp_key + '.')? Otherwise if, for some reason, we have a node named valve1.valve2.node, it will be identified as belonging to valve2.

You're right, that was the idea I was going for.

application/vis_area_res/visualization_area.py, line 135 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

bounding again

Done.

application/vis_area_res/visualization_area.py, line 144 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Extra space at the end of this line (nit)

Done.

application/vis_area_res/visualization_area.py, line 145 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

remove newline

Done.

application/vis_area_res/visualization_area.py, line 169 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

This is a very long function, and it's quite difficult to read as a result. Please consider splitting it up into several smaller functions performing discrete steps (setting up fonts/pens/brushes, drawing the grid, drawing nodes and edges, etc.)

Eventually, my goal is to delegate drawing to the elements on the board, which will shrink the function significantly. I haven't implemented that yet (There is actually an empty function for that in graphics_component). Could I keep it as-is in the current PR, since overhauling drawing delegation is a large task that I plan to do in the next update?

application/vis_area_res/visualization_area.py, line 210 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        num_horiz_lines = 10
        num_vertical_lines = 10

Extract as constants

Done.

application/vis_area_res/visualization_area.py, line 212 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

        vertical_interval = self.initial_bounds.height() / num_vertical_lines
        horiz_interval = self.initial_bounds.width() / num_horiz_lines
Swap these two lines so they're in the same order as the previous two (horizontal and then vertical)

Done.

application/vis_area_res/visualization_area.py, line 216 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Also for the sake of consistency, might be better to have in range(num_vertical_lines). If this will always be the same as num_horizontal_lines it could be helpful to combine them too, since changing multiple hard coded values later on is a pain

Yeah, I see the issue here. I'm just going to split into two for loops for a potential future version where the numbers differ.

application/vis_area_res/visualization_area.py, line 251 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Extra space between "precedence" and "of" (nit)

Done.

application/vis_area_res/visualization_area.py, line 267 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

(p2[0] - p1[0]) == 0:
p1[0] == p2[0]

Done.

application/vis_area_res/visualization_area.py, line 270 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

((p2[0] - p1[0]))
remove extra parens

Done.

application/vis_area_res/visualization_area.py, line 281 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

                painter.drawLine(p1[0] + (self.NODE_RAD + 1)*cos(theta_edge),
                                 p1[1] + (self.NODE_RAD + 1)*sin(theta_edge),
                                 p2[0] - (self.NODE_RAD)*cos(theta_edge), p2[1]
                                 - (self.NODE_RAD)*sin(theta_edge))

Why only adding 1 for the first point?

I honestly don't really know, but the renderer tends to extend it past the circle on the first point. I've just tuned it by trial-and-error.

application/vis_area_res/visualization_area.py, line 411 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

qml -> QML

Done.

application/vis_area_res/visualization_area.py, line 474 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

extra whitespace at the end of the line

Done.

application/vis_area_res/visualization_area.py, line 502 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

This function should probably be much further up in this class; right now it's in the middle of some QML functions, and it doesn't really fit here

That's actually a good point, it should probably live closer to scale_engine_instance

application/vis_area_res/visualization_area.py, line 504 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Sets

Set

Done.

application/vis_area_res/visualization_area.py, line 512 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

plumbing_engine
PlumbingEngine

Done.

application/vis_area_res/visualization_area.py, line 552 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

I really like this idea - will be very useful once we add our own logging system.

To be honest, I just wanted a print statement in QML when I wrote this.

application/vis_area_res/visualization_area.py, line 557 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

python
Python

Done.

application/vis_area_res/visualization_area.py, line 572 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

froce -> force

Done.

application/vis_area_res/visualization_area.py, line 705 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Demo sprite accessing
End with period

Done.

jacobdeery

Reviewable status: 0 of 17 files reviewed, 23 unresolved discussions (waiting on @CODE-TITAN and @jacobdeery)

application/plumbing_vis/graphics_component.py, line 20 at r5 (raw file):

            print(f'could not load "{component_type}" icon from' +
                    'application/resources/component_icons/ !')

Print the actual filepath that fails to load, not the directory. Make it a variable so that it's guaranteed to be the same between the QImage call and the debug print

application/plumbing_vis/graphics_component.py, line 22 at r5 (raw file):

sucessful

successful

application/plumbing_vis/visualization_area.py, line 13 at r5 (raw file):

# NOTE: Copied from layout_demo to avoid using importlib

Remove at least one of these newlines

application/plumbing_vis/visualization_area.py, line 132 at r5 (raw file):

        t = self.terminal_graph
        pos = self.layout_pos

No need for these since the variables are only used once

application/plumbing_vis/visualization_area.py, line 155 at r5 (raw file):

        for comp_name_key in self.graphics_components.keys():
            comp = self.graphics_components[comp_name_key]

If you only need the key for accessing the value, you can just iterate over the values instead

application/plumbing_vis/visualization_area.py, line 169 at r5 (raw file):

        t = self.terminal_graph
        pos = self.layout_pos

No need for these

application/plumbing_vis/visualization_area.py, line 175 at r5 (raw file):

            if self.DEBUG_MODE:
                print('node: ' + str(pt[0]) + str(pt[1]))

Make this debug message more descriptive ("terminal graph has node {node} at coordinates ({pt[0]}, {pt[1]})")

Add space between x and y values

application/plumbing_vis/visualization_area.py, line 266 at r5 (raw file):

                if self.DEBUG_MODE:
                    pt = pos[node]
                    print('node: ' + str(pt[0]) + str(pt[1]))

More descriptive, and add space between x and y values

application/plumbing_vis/visualization_area.py, line 296 at r5 (raw file):

                    print('edge end 1: ' + str(p1[0]) + str(p1[1]))
                    print('edge end 2: ' + str(p2[0]) + str(p2[1]))

Add spaces between the x and y values. Consider using an f-string for better readability

application/plumbing_vis/visualization_area.py, line 478 at r5 (raw file):

        for n_key, node in self.graphics_nodes.items():
            if node.contains(position_float):
                print('hover detected: ' + str(node.x()) + ' ' + str(node.x()))

only print in debug mode?

application/plumbing_vis/visualization_area.py, line 714 at r5 (raw file):

application/resources/component_icons/parrot.jpg

This path also needs a find_resource

application/resources/component_icons/parrot.jpg, line at r1 (raw file):

Previously, Code-Titan wrote…

I've checked and it's a Getty Images image: "Photo: Francois Nel/Getty Images". Which means that we can't use it in any sort of distributed project. It's meant as a placeholder - as soon as we get a proper sprite sheet we can change it up.

Can you confirm that this has been swapped out for an image with a permissive license?

application/resources/component_icons/valve_icon.png, line at r1 (raw file):

Previously, Code-Titan wrote…

That's a logo, so it will definitely have to go when we have a proper sprite sheet.

Same as above

application/vis_area_res/graphics_component.py, line 20 at r1 (raw file):

Previously, Code-Titan wrote…

print(f'could not load "{component_id}"!')
I didn't know you could do that in Python, thanks!

As for the debug, all of them currently search the same folder, so I'm not sure if it's useful to say which every time.
Nonetheless, I'll add a line that says which folder was searched.

The point is to print the filename, not the directory

application/vis_area_res/visualization_area.py, line 8 at r1 (raw file):

Previously, Code-Titan wrote…

For basic mathematical operators, does it really matter where they are being imported from? I chose numpy rather arbitrarily since I have some experience with it in the past.

Numpy is fine, but it's typically good practice to just import the namespace and then explicitly reference np.sin, np.pi, etc.. That way, if you decide you need something else from numpy (like sqrt), you can just use np.sqrt instead of needing to go import it again.

application/vis_area_res/visualization_area.py, line 90 at r1 (raw file):

Previously, Code-Titan wrote…

How much the window is panned by (in total for 'permanent', and during the current drag operation for 'temporary'). I'll add a comment describing this, and I'll also change it to 'factor' to fit with the sentiment of the past comment.

"transpose" is probably not the right term, then; something like temporary_x_translation is probably more accurate

application/vis_area_res/visualization_area.py, line 169 at r1 (raw file):

Previously, Code-Titan wrote…

Eventually, my goal is to delegate drawing to the elements on the board, which will shrink the function significantly. I haven't implemented that yet (There is actually an empty function for that in graphics_component). Could I keep it as-is in the current PR, since overhauling drawing delegation is a large task that I plan to do in the next update?

Please leave a TODO for this

application/vis_area_res/visualization_area.py, line 281 at r1 (raw file):

Previously, Code-Titan wrote…

I honestly don't really know, but the renderer tends to extend it past the circle on the first point. I've just tuned it by trial-and-error.

Please leave a comment explaining this

Artem-Sotnikov

Reviewable status: 0 of 17 files reviewed, 23 unresolved discussions (waiting on @CODE-TITAN and @jacobdeery)

application/resources/component_icons/parrot.jpg, line at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Can you confirm that this has been swapped out for an image with a permissive license?

Yes, it has been swapped.

application/resources/component_icons/valve_icon.png, line at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Same as above

Yep.

jacobdeery

Running the application results in two deprecation warnings and two errors:

file:///D:/Documents/Rocketry/topside/application/resources/PlumbingPane.qml:133:9: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... }
file:///D:/Documents/Rocketry/topside/application/resources/PlumbingPane.qml:29:9: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... }
file:///D:/Documents/Rocketry/topside/application/resources/PlumbingPane.qml:27: TypeError: Property 'test' of object VisualizationArea(0x27804ba0820) is not a function
file:///D:/Documents/Rocketry/topside/application/resources/PlumbingPane.qml:15: ReferenceError: test is not defined

Please resolve all of these as well

Reviewable status: 0 of 17 files reviewed, 21 unresolved discussions (waiting on @CODE-TITAN and @jacobdeery)

Artem-Sotnikov added 2 commits July 8, 2020 13:23

app: Publish updated UI wroking version

c391ddd

Code is a working version that is missing comments and formatting. However, several new UI features are functional and the groundwork for adding more functionality is complete.

app: Clean and optimize code

6f0bdc8

All methods and classeshave been cleaned and commented. Unecessary files were removed. Moreover, several features have been added: - action freeze upon context menu opening - selective context menu requests - parrot mode demo

Artem-Sotnikov requested review from jacobdeery and wendi-yu July 12, 2020 01:04

wendi-yu requested changes Jul 12, 2020

View reviewed changes

wendi-yu reviewed Jul 12, 2020

View reviewed changes

jacobdeery reviewed Jul 12, 2020

View reviewed changes

wendi-yu and others added 8 commits July 11, 2020 21:18

plumbing: Add component accessor method (#48)

b0dd04e

Also add copy protection for the errors accessor, to keep changes from propagating through to the engine's sets/dicts.

app: Address some of the issues before rebase

bee47d1

Some issues that have come up have been fixed.

Stop trying to format deleted files (#49)

feeefa1

Add a diff-filter to the git diff to exclude deleted files.

app: Publish updated UI wroking version

e880ba7

Code is a working version that is missing comments and formatting. However, several new UI features are functional and the groundwork for adding more functionality is complete.

app: Clean and optimize code

09e0c43

All methods and classeshave been cleaned and commented. Unecessary files were removed. Moreover, several features have been added: - action freeze upon context menu opening - selective context menu requests - parrot mode demo

app: Address some of the issues before rebase

34f1005

Some issues that have come up have been fixed.

Merge branch 'visualization_area_update_1' of https://github.com/wate…

cf2b501

…rloo-rocketry/topside into visualization_area_update_1

app: Work on addressing current PR issues

b257971

Artem-Sotnikov commented Jul 12, 2020

View reviewed changes

jacobdeery reviewed Jul 12, 2020

View reviewed changes

Artem-Sotnikov commented Jul 12, 2020

View reviewed changes

jacobdeery reviewed Jul 12, 2020

View reviewed changes

jacobdeery added the application Affects the Topside application label Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualization area update 1 #47

Visualization area update 1 #47

Artem-Sotnikov commented Jul 12, 2020 •

edited by jacobdeery

wendi-yu left a comment

wendi-yu left a comment

jacobdeery left a comment

Artem-Sotnikov left a comment

jacobdeery left a comment

Artem-Sotnikov left a comment

jacobdeery left a comment

Visualization area update 1 #47

Are you sure you want to change the base?

Visualization area update 1 #47

Conversation

Artem-Sotnikov commented Jul 12, 2020 • edited by jacobdeery

wendi-yu left a comment

Choose a reason for hiding this comment

wendi-yu left a comment

Choose a reason for hiding this comment

jacobdeery left a comment

Choose a reason for hiding this comment

Artem-Sotnikov left a comment

Choose a reason for hiding this comment

jacobdeery left a comment

Choose a reason for hiding this comment

Artem-Sotnikov left a comment

Choose a reason for hiding this comment

jacobdeery left a comment

Choose a reason for hiding this comment

Artem-Sotnikov commented Jul 12, 2020 •

edited by jacobdeery