Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#25-Transparency #193

Merged
merged 34 commits into from
Sep 12, 2021
Merged

#25-Transparency #193

merged 34 commits into from
Sep 12, 2021

Conversation

flooie
Copy link
Contributor

@flooie flooie commented Sep 4, 2021

PR Adds support for transparent pen, rect and ellipses.

I think this is more of a Draft than an actual PR, but I think it could be helpful to get some quick pointers.

It enables support for transparency, and while rather straightforward I do see some potential improvements, in both tool icon generation - tool color and toggling.

@edemaine edemaine linked an issue Sep 4, 2021 that may be closed by this pull request
5 tasks
Bug flipped if boolean
to avoid dots appearing
in highlights
Generate opacity icons in a loop
Cleanup the proof of concept into
a cleaner working code
Cleanup more superflous code.
Its amazing how you can see the excess on the third or fourth commit.
It just dawned on me that these variable
are really not useful anymore
allow25Percent
allow50Percent
allow75Percent
@flooie
Copy link
Contributor Author

flooie commented Sep 5, 2021

@edemaine I see you linked #25 but with a few changes this might also be able to tackle #173 ?

@flooie
Copy link
Contributor Author

flooie commented Sep 5, 2021

@edemaine -- also, I've kept changing the PR all day, but I think I have it where I'm now ready for you to tear it apart.

Copy link
Owner

@edemaine edemaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and sorry for the delay in reviewing! This is definitely on the right track, and is probably pretty close to being done. But I have a bunch of comments to do things in more Cocreate ways. If you'd rather do [some of] them, e.g. it's unclear what I mean, let me know.

'opacity50':
'<circle cx="250" cy="250" r="200" fill-opacity="0.50"/>'
'opacity75':
'<circle cx="250" cy="250" r="200" fill-opacity="0.75"/>'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to draw these in the tool itself, like we do with line width.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. √

click: (e) -> selectColorOrFill e, color
click: (e) ->
selectColorOrFill e, color
updateColorOpacity()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should't need an explicit update call here; should use reactivity instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -15,6 +17,8 @@ export dark = new storage.Variable 'dark', false

Tracker.autorun ->
dom.classSet document.body, 'dark', dark.get()
allowTransparency.set false
currentOpacity.set 1.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost certainly the wrong place for this code. Tracker.autorun reruns this code whenever the dependencies (here, dark.get()) change. I don't think you want to reset these things whenever dark mode changes.

@@ -81,3 +85,44 @@ defineTool
click: ->
return unless (room = currentRoom.get())?
room.gridSnap.set not room.gridSnap.get()

defineTool
name: 'Transparency'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lower case for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -53,14 +55,15 @@ export class RenderObjects
class: 'pen'
,
dataset: id: id
if simple
if simple or allowTransparency.valueCur != true
Copy link
Owner

@edemaine edemaine Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and test above) seems wrong: you're using simple rendering whenever the transparency tool is currently active. To see this fail, do some transparent drawing, and then reload the board: everything will be opaque. You should instead be looking at whether the object itself has transparency.

Also, should document that pressure sensitivity doesn't work with transparency mode. I think that's probably better than the messy dots along the way (until we can support actual transparency with varying line thickness, via <path> rendering).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep ... I didn't quite understand what was going on in my first pass but I knew that the darn dots were ruining my beautiful transparent stroke.



# These values are chosen for no particular reason. I saw that
# 12.5 was a number you liked for highlighting Perhaps .25 should be 12.5
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you see 12.5? Ah, 0x20 in #139. That's not me, but may be useful guideline. I'll need to experiment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave this to your wysdom

allowTransparency.set not allowTransparency.get()
updateOpacity 1.0, allowTransparency.curValue
init: ->
updateOpacity 1.0, false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calls to updateOpacity should be replaced with just setOpacity.


buttons = document.querySelectorAll('[data-tool^="Opacity"]')
for button in buttons
button.style.display = if display then "block" else "none"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the move to React, we don't want to manipulate widget DOM directly like this. Instead, add to DrawApp something like:

transparency = useTracker -> allowTransparency.get()
...
{if transparency
  <div id="opacities" className="subpalette">
    <ToolCategory category="opacity" placemen="top"/>
  </div>
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for making that easier for me

allowTransparency.set display

if currentOpacity.get() == val
currentOpacity.set 1.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like resetting currentOpacity to 1 when toggling the mode off. I'd rather keep the same opacity when I leave and re-enter transparent mode. Similar to how fill mode works, we should have a Boolean currentOpacityOn, which should replace the current allowTransparency.

Similarly, I don't like that clicking on the transparent mode for the first time doesn't immediately turn on transparency. It (and thus currentOpacity) should start at a global default (maybe 50%?), so one of the specific buttons starts down as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bye bye allowTransparency. √

@@ -44,11 +44,13 @@ Meteor.methods
pts: [xywType]
color: String
width: Number
opacity: Number
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably be made optional in the same way as fill.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did it right but I basically aped some other code

@flooie
Copy link
Contributor Author

flooie commented Sep 11, 2021

@edemaine I think I hit most or all of your changes. Thanks - it should be much more cocreate-y

@edemaine
Copy link
Owner

@flooie Please push your changes...

@flooie
Copy link
Contributor Author

flooie commented Sep 11, 2021

@edemaine lol - thats embarrassing.

@edemaine edemaine removed a link to an issue Sep 12, 2021
5 tasks
@edemaine edemaine merged commit d0f79ad into edemaine:main Sep 12, 2021
@edemaine
Copy link
Owner

Thanks @flooie for working on this! I'm excited to play around with it (and we'll see whether 25, 50, 75 are the right options in practice). It'll be deployed within the next ~30 minutes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants