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

Reference Images #771

Merged
merged 5 commits into from
Nov 8, 2022
Merged

Conversation

20kdc
Copy link
Contributor

@20kdc 20kdc commented Oct 27, 2022

Affects #565.

Usecase:

A friend of mine was looking for a pixel editor that was, well, not the one we've all heard of, and considers high-resolution reference images to be critical to her rotoscoping workflow.

Images:

image

Usage:

  • New reference image is a new image open type.
  • Reference images are not stored in the project file due to size reasons.
  • Reference images may be scaled and positioned in project space in a newly added panel (currently appearing as a tab of Tools, which is unfortunate)

TODO:

  • Figure out how to make it work with the UI layout
  • Anything I forgot (it appears that Pixelorama code requires changing things in a lot of places, and it's possible I forgot something)

Rationale of implementation decisions:

Layers were not used as the state of the code makes this difficult.

Neither BaseCel or BaseLayer are capable of providing custom render logic (which seems odd - even if GroupCel did nothing as part of what appears to be a general 'flattening' of cels, it would still be pertinent to have the cels perform rendering by themselves given the clear intent of the design)

In addition, the assumption would be that reference layers probably do not want to be disposable cels (as a new blank frame would cause the reference image to disappear, which is not necessarily wanted)

Guides on the other hand have complete control of their rendering, and therefore I chose the design of guides as the basis of the feature.

Reference images are expected to be large and probably in a format such as PNG or JPEG. Therefore reference images are linked by absolute file path. This is up for potential change but doing so could be hazardous.

It's unclear how exactly changes are expected to propagate - I presently assume project_changed is the global "anything important was modified" function, and this appears to work reliably enough.

@mrtripie
Copy link
Contributor

This looks great!
project_changed is called when changing between projects (ie: opening a new one or changing project tabs).

Custom cel render logic is intended to be placed in the update_texture method of classes extending BaseCel. image_texture should be updated there, and in Canvas.gd the image_texture of each cel in the current Frame are blended together.

PixelCel converts its image (used on the CPU) to image_texture (used on the GPU) there.

I'm working on a vector layers branch as another example: https://github.com/mrtripie/Pixelorama/blob/VectorLayers/src/Classes/VectorCel.gd (near the bottom)

Currently GroupCels don't flatten their children cels, but probably will some time in the future.

I agree that your current approach is better than having it as a layer though. You should be able to do things like display the reference off to the side of the canvas right?

@20kdc
Copy link
Contributor Author

20kdc commented Oct 27, 2022

Custom cel render logic is intended to be placed in the update_texture method of classes extending BaseCel. image_texture should be updated there, and in Canvas.gd the image_texture of each cel in the current Frame are blended together.

This would definitely be unusable for reference purposes as this is sort of implying that it'll show up in the export

I agree that your current approach is better than having it as a layer though. You should be able to do things like display the reference off to the side of the canvas right?

You can in fact display the reference to the side of the canvas, yes

@mrtripie
Copy link
Contributor

mrtripie commented Oct 27, 2022

This would definitely be unusable for reference purposes as this is sort of implying that it'll show up in the export

The cels are also blended together when exporting, so reference layers could be excluded in export. (Canvas's blending is just for seeing the end result in Pixelorama, I think it's also blended together separately in the small preview panel usually in the top right)

If it used layers, it would be restricted to the project's resolution, and the boundary of the Canvas though.

@20kdc
Copy link
Contributor Author

20kdc commented Oct 27, 2022

If it used layers, it would be restricted to the project's resolution, and the boundary of the Canvas though.

Yeah, that definitely... would not have been fit for the purpose

@OverloadedOrama
Copy link
Member

Overall this looks good and it seems to be working well. I have some comments about the UI before merging though.

  1. Instead of SpinBox nodes for the reference image properties (ReferenceImageButton.tscn), we should use the custom ValueSlider node instead. It's a node introduced in v0.11 and it can be found under src/UI/Nodes/ValueSlider.tscn. It essentially combines a spinbox and a slider, allowing for easier moving around/scaling the reference image, without necessarily having to put numeric values every time we want to make a change.

  2. In the ReferencesPanel, the Label should have autowrap enabled. Without it, the panel cannot be resized to a size smaller than the width of the text. This is very likely to cause issues with smaller screens and localization, where the translated text may be even bigger than the English one.

  3. In ReferenceImageButton.tscn, I noticed that the Opacity option allows for values smaller than 0 and bigger than 100. Is there any reason for this?

@20kdc
Copy link
Contributor Author

20kdc commented Nov 7, 2022

While handling these: While there are no conflicts, should I rebase anyway?

@OverloadedOrama
Copy link
Member

Rebasing is not necessary since there are no conflicts.

@20kdc
Copy link
Contributor Author

20kdc commented Nov 7, 2022

e28fbba fixes all three of these:

1. Instead of SpinBox nodes for the reference image properties (ReferenceImageButton.tscn), we should use the custom ValueSlider node instead. It's a node introduced in v0.11 and it can be found under `src/UI/Nodes/ValueSlider.tscn`. It essentially combines a spinbox and a slider, allowing for easier moving around/scaling the reference image, without necessarily having to put numeric values every time we want to make a change.

Done.

2. In the ReferencesPanel, the Label should have autowrap enabled. Without it, the panel cannot be resized to a size smaller than the width of the text. This is very likely to cause issues with smaller screens and localization, where the translated text may be even bigger than the English one.

Autowrap is now enabled on all the references labels.

3. In ReferenceImageButton.tscn, I noticed that the Opacity option allows for values smaller than 0 and bigger than 100. Is there any reason for this?

There wasn't, I've fixed it.

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

Should be good to merge now. Thank you very much for the contribution!

@OverloadedOrama OverloadedOrama merged commit 20bce81 into Orama-Interactive:master Nov 8, 2022
@20kdc
Copy link
Contributor Author

20kdc commented Nov 8, 2022

Thank you, also.

@20kdc
Copy link
Contributor Author

20kdc commented Nov 8, 2022

...So I mentioned in the TODO in the PR description:

  • Figure out how to make it work with the UI layout

I have noticed that no further changes were made to the PR to facilitate doing this properly, so I'm assuming you're handling it on your end on master?

@OverloadedOrama
Copy link
Member

From my testing, it seems to already work well with the UI layout system. There was a crash when dropping the References tab into the main canvas, but this is unrelated to this PR and it got fixed when updating the addon to version 1.1.1 (927e4f4).

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.

3 participants