qmk / qmk_configurator

The QMK Configurator
http://config.qmk.fm
689 stars 343 forks source link

Key shape simplified #260

Open i5ar opened 5 years ago

i5ar commented 5 years ago

I have a few suggestions regarding the key shape as defined in the docs.

  1. The key shape is defined as a polyline (open path) so the first [x, y] pair is always equal to the last one. It's better to define the key shape as a polygon (closed path) so the last [x, y] pair can be removed;
  2. The key shape is defined as a nested array but in this case I think it's better to use a simple array since the path will be generally short and therefore readable without nesting [x, y] pairs. This is also how the polygon is defined in SVG (i.e. <polygon points="-25, 0, 125, 0, 125, 200, 0, 200, 0, 100, -25, 100" />);
  3. The origin for the key shape should not be in the top-left but in the same position used by the rectangular key. This will make it easier to design and maintain the configurator because the origin will always be in the same spot with or without a key shape value.

Maybe it's too late or maybe you don't like this approach but if you take this road I also suggest using the key p (points) to avoid breaking changes with ks.

In conclusion the ISO Enter key would look like:

"p": [-0.25, 0, 1.25, 0, 1.25, 2, 0, 2, 0, 1, -0.25, 1]

Instead of:

"ks": [[0, 0], [1.5, 0], [1.5, 2], [0.25, 2], [0.25, 1], [0, 1], [0, 0]]

Thank you.

yanfali commented 5 years ago

Great suggestions. So right now everything is defined as divs to facilitate drag and drop. I did experiment with svg here

https://yanfali.github.io/keycodes/

The issue being interaction with canvas vs DOM. They don't really mix all that nicely. I need to revisit the rendering algorithm using svg elements rendered inside DOM.

The coordinate system is actually in key units and is converted to pixels. We actually need to support rotation to so at some point this does need revisiting.

The nesting is part of the API. It's logically each row, I think we inherited that from kle.

Thank you for the feedback, it's appreciated.

yanfali commented 5 years ago

Please check out the discussion https://github.com/qmk/qmk_firmware/issues/2621 and feel free to contribute your feedback. Info.json is definitely going to evolve. It's not just used for rendering though. It's part of the API interaction between UI and qmk

i5ar commented 5 years ago

The coordinate system is actually in key units and is converted to pixels.

That's perfect. In my suggestion I only changed the structure of the array and the position of the relative origin from here:

○----+
|    |
+-+  |
  |  |
  +--+

to here:

+-○--+
|    |
+-+  |
  |  |
  +--+

We actually need to support rotation to so at some point this does need revisiting.

I'm building something but I haven't tried to support rotation yet. If I have time, I'll try to implement something this night, just to make sure this approach works nicely with more complex layouts.

yanfali commented 5 years ago

Very neat. @harshitgoel was experimenting with that a month or so ago, but I think he got busy. Where to do the rotation is a matter of contention. KLE is our reference implementation as they do currently support rotation and one of the sources of layout information is KLE. One key feature or our current implementation is drag-n-drop so it's important to not pick technologies that prevent that mode of operation.

i5ar commented 5 years ago

I wouldn't use KLE as reference for rx and ry. Rotation r works fine but rx and ry have some strange behavior.

I have to apologize: The implementation for rx and ry in KLE is correct. The only thing to keep in mind is that in QMK (info.json) x and y values are incremented for each key while in KLE (keyboard-layout.json) x and y values do not increment.

To prove the suggestions above work nicely I made an example.

i5ar commented 5 years ago

I investigated a little more into the KLE docs and I can confirm that KLE is using the same origin I proposed for oddly-shaped keys, as you can see here.

Basically this one:

+-○--+
|    |
+-+  |
  |  |
  +--+
skullydazed commented 5 years ago

x and y in KLE is a different concept. In KLE x and y default to 1 and 0 respectively and represent the offset from the previous key. In QMK those are an absolute coordinate. Statelessness is a key feature of our format, and is what drives most of our deviations from KLE. As such you can't compare them this way.

It's interesting that KLE draws ISO enter the way it does, and this has been convenient for our early work as iso enter has "just worked" with a 1.25x2 rectangle. I think the ISO Enter change you're proposing is simply a change to convention, not a change in implementation, and probably makes sense. We aren't using it anywhere yet AFAIK, so changing this now is pretty easy. Thanks for helping us to see this necessity of this!

I see your point (no pun intended) about collapsing point pairs into a single list rather than nested but I'm not a fan of that approach because it makes working with the format harder if you're not intimately familiar with it or svg, and most users of QMK are not experts. Plus, nesting them this way lets you use code formatting tools to break them out into lines for easy and convenient editing.

yanfali commented 5 years ago

I actually think there's a way to do an ISO key using pure CSS using :after and :before selectors, I just haven't had time to look into it.

yanfali commented 5 years ago

@i5ar if you'd like to look at doing ISO key support in pure CSS I would be most appreciative

i5ar commented 5 years ago

KLE is a different concept [...] Statelessness is a key feature of our format, and is what drives most of our deviations from KLE.

@skullydazed I realized that today as I found the docs of KLE in the Wiki. As a matter of fact, I had a hard time explaining how I made the rotation work when using rx and ry (the example is above).

It's interesting that KLE draws ISO enter the way it does, and this has been convenient for our early work as iso enter has "just worked" with a 1.25x2 rectangle.

:smile: Exactly!

I see your point (no pun intended) about collapsing point pairs into a single list rather than nested but I'm not a fan of that approach because it makes working with the format harder if you're not intimately familiar with it or svg, and most users of QMK are not experts.

About collapsing point pairs into a single list:

Also, some JSON formatter tools do this boring expansion:


          "ks": [
            [
              0,
              0
            ],
            [
              1.5,
              0
            ],
            [
              1.5,
              2
            ],
            [
              0.25,
              2
            ],
            [
              0.25,
              1
            ],
            [
              0,
              1
            ],
            [
              0,
              0
            ]
          ],

Just something to consider that I wanted to point out. Thank you.

i5ar commented 5 years ago

@yanfali This should work.

<div id="enter"></div>

<style>
#enter {
  background: teal;
  height: 200px;
  position: relative;
  width: 125px;
}
#enter:after {
  background: teal;
  content: "";
  height: 100px;
  left: -25px;
  top: 0;
  position: absolute;
  width: 25px;
}
</style>

Of course, you need to scale the pixels accordingly with the configurator.

yanfali commented 5 years ago

Awesome, thanks. I'll take a look when I get a chance. I may be a few days.

yanfali commented 5 years ago

@i5ar I gave it a shot, but it didn't really work. I'm thinking that we may need to use 3 divs to represent this one key with the appropriate coloring and styling.

i5ar commented 5 years ago

@yanfali This is how much further I could go https://jsfiddle.net/yzq2m6pc/3/ from here https://jsfiddle.net/zqg78yeb/

Those are two different approaches (both need adjustments). I didn't change the HTML, which I copied from the configurator (gh60).

UPDATE: Last attempt: https://jsfiddle.net/n3uxace8/