Implemented rendering for OrGate #1237

rren8957 · 2023-03-23T14:46:44Z

…o use BaseShapePrim

LeonMontealegre

Awesome start!
There appears to be quite a few visual issues, however:

LeonMontealegre · 2023-03-24T21:31:00Z

src/app/core/internal/view/rendering/prims/QuadCurve.ts

        const p1 = this.p1;
        const p2 = this.p2;
        const c  = this.c;


Let's changed this to const { p1, p2, c } = this; -- more succinct !

LeonMontealegre · 2023-03-24T21:33:07Z

src/app/digital/internal/views/DigitalCircuitView.ts

@@ -24,6 +25,7 @@ export class DigitalCircuitView extends CircuitView {
        if (!this.assemblers) {
            this.assemblers = {
                "ANDGate": new ANDGateAssembler(this.circuit, this, this.selections, this.sim),
+                "ORGate": new ORGateAssembler(this.circuit, this, this.selections, this.sim),


nit: put an extra space before the new so that it's vertically aligned with the others

LeonMontealegre · 2023-03-24T21:33:21Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+import {SelectionsManager}    from "core/internal/impl/SelectionsManager";
+import {CircuitView}          from "core/internal/view/CircuitView";
+import {PortAssembler}        from "core/internal/view/PortAssembler";
+import {QuadCurve}                 from "core/internal/view/rendering/prims/QuadCurve";


nit: spacing

LeonMontealegre · 2023-03-24T21:33:39Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+import {DigitalComponentInfo} from "digital/internal/DigitalComponents";
+import {DigitalSim}           from "digital/internal/sim/DigitalSim";
+import {Assembler}            from "core/internal/view/Assembler";
+


nit: need 2 blank lines between imports and code

LeonMontealegre · 2023-03-24T21:39:58Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+            const transform = this.view.componentTransforms.get(gate.id)!;
+
+            const selected = this.selections.has(gate.id);


we can put these outside of the loop since they don't depend on i

…rcuits/OpenCircuits into renr_ORGateRendering

It's always rendered blurrily on my end for some reason so I could have missed some other visual issues.

LeonMontealegre

So close but it's still not working for 7/8 inputs!

Definitely try and steal from the old code as much as possible, you shouldn't need to stray from it at all, and lmk if you're encountering problems with that!

Great job so far though !

LeonMontealegre · 2023-04-13T21:03:49Z

src/app/digital/internal/views/DigitalCircuitView.ts

                "ANDGate": new ANDGateAssembler(this.circuit, this, this.selections, this.sim),
+                "ORGate":  new ORGateAssembler(this.circuit, this, this.selections, this.sim),


please re-order the assemblers as they were

LeonMontealegre · 2023-04-13T21:47:25Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+            "outputs": () => ({ origin: V(0.5, 0), dir: V(1.1, 0) }),
+            "inputs":  (index, total) => {
+                const spacing = 0.5 - this.options.defaultBorderWidth/2;
+                return { origin: V(-0.43, spacing*((total-1)/2 - index)), dir: V(-1.3, 0), stroke: {color: "black", size: 1, lineCap: "round"}


Format on multiple lines please, like:

return { origin: V(...), dir: V(...), stroke: {...} };

LeonMontealegre · 2023-04-13T21:49:10Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+            },
+        });
+
+        this.portAssemblerV2 = new PortAssembler(circuit, view, selections, {


Please see the old code for all the math for the positioners, it shouldnt be this complicated, and you shouldn't need two different ones

https://github.com/OpenCircuits/OpenCircuits/blob/master/src/app/digital/models/ports/positioners/QuadraticCurvePositioner.ts#L27

LeonMontealegre · 2023-04-13T21:49:39Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+
+            const c = V(-l, d);
+
+            if (numInputs <= 3) {


why is this necessary ?

LeonMontealegre · 2023-04-13T21:50:30Z

src/app/digital/internal/views/components/ORGateAssembler.ts

+        }
+
+        const numInputs = this.getNumInputs(gate);
+        if (numInputs % 2 === 0)


You shouldn't need two positioners, you can use total in the positioner to check if it's even or odd and return a different position based on that

Fixed visual issues for input port of size 7 and 8. I didn't exactly used the old code but I did simplify the code to only have 1 positioner/portAssembler.

The base branch was changed.

…OpenCircuits into renr_ORGateRendering

rren8957 added 3 commits March 21, 2023 23:57

Finished rendering ORGate

dbc86b0

Added function to set number of ports

fb7895f

OrGate now renders correctly for multiple inputs & Updated QuadGate t…

7f1d869

…o use BaseShapePrim

rren8957 requested a review from LeonMontealegre March 23, 2023 14:47

github-actions bot added the Status: Awaiting Review label Mar 23, 2023

LeonMontealegre requested changes Mar 24, 2023

View reviewed changes

github-actions bot added Status: Changes Requested and removed Status: Awaiting Review labels Mar 24, 2023

rren8957 added 5 commits March 24, 2023 21:25

Merge branch 'model_refactor_api_views1' of https://github.com/OpenCi…

1ddd257

…rcuits/OpenCircuits into renr_ORGateRendering

Updated code to reflect style refactor

9d7ff7d

Merge branch 'model_refactor_api_views1' of https://github.com/OpenCi…

0d9a3e3

…rcuits/OpenCircuits into renr_ORGateRendering

Small changes

3863257

Fixed Visual Issues?

fb6137b

It's always rendered blurrily on my end for some reason so I could have missed some other visual issues.

rren8957 requested a review from LeonMontealegre April 4, 2023 23:53

github-actions bot added Status: Awaiting Review and removed Status: Changes Requested labels Apr 4, 2023

LeonMontealegre requested changes Apr 13, 2023

View reviewed changes

github-actions bot added Status: Changes Requested and removed Status: Awaiting Review labels Apr 13, 2023

Fixed Visual issues p2/made requested changes

9f5d2c2

Fixed visual issues for input port of size 7 and 8. I didn't exactly used the old code but I did simplify the code to only have 1 positioner/portAssembler.

rren8957 requested a review from LeonMontealegre April 13, 2023 23:01

github-actions bot added Status: Awaiting Review and removed Status: Changes Requested labels Apr 13, 2023

LeonMontealegre previously approved these changes Apr 14, 2023

View reviewed changes

github-actions bot added Status: Ready to Merge and removed Status: Awaiting Review labels Apr 14, 2023

LeonMontealegre changed the base branch from model_refactor_api_views1 to model_refactor_api May 31, 2023 02:55

Merge branch 'model_refactor_api' of https://github.com/OpenCircuits/…

9559618

…OpenCircuits into renr_ORGateRendering

LeonMontealegre approved these changes May 31, 2023

View reviewed changes

LeonMontealegre merged commit bae2e90 into model_refactor_api May 31, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented rendering for OrGate #1237

Implemented rendering for OrGate #1237

rren8957 commented Mar 23, 2023

LeonMontealegre left a comment

LeonMontealegre Mar 24, 2023

LeonMontealegre Mar 24, 2023

LeonMontealegre Mar 24, 2023

LeonMontealegre Mar 24, 2023

LeonMontealegre Mar 24, 2023

LeonMontealegre left a comment

LeonMontealegre Apr 13, 2023

LeonMontealegre Apr 13, 2023

LeonMontealegre Apr 13, 2023

LeonMontealegre Apr 13, 2023

LeonMontealegre Apr 13, 2023

		const transform = this.view.componentTransforms.get(gate.id)!;

		const selected = this.selections.has(gate.id);

		"ANDGate": new ANDGateAssembler(this.circuit, this, this.selections, this.sim),
		"ORGate": new ORGateAssembler(this.circuit, this, this.selections, this.sim),

Implemented rendering for OrGate #1237

Implemented rendering for OrGate #1237

Conversation

rren8957 commented Mar 23, 2023

LeonMontealegre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeonMontealegre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment