UV-mapping issues

[peteihis]

There are currently several things wrong with the Texture UV-Mapping of PME.

The expected procedure:

• Mark seams and unfold
• Export the map
• Paint a texture on an image processor over the map.
• Import the image as texture.
• Call it mapped.
• Render.

What now happens:

• The user has to set the new texture 'UV-mapped' and 'faces mapped individually' for the mapping to work. This should happen through the UV-editor of PME pretty much automatically.
• The exported image is misscaled, so the painted texture does not match the unfolded mesh. The misscaling can be avoided, by closing and reopening the mapping editor and then exporting without changing the dialog size before the export, but this level of wit and patience should not be expected from the user.
• The dialog does not show the size of the (to-be) exported area to the user.
• There should be a visualized preset for the image ratio or the ratio should be locked. Now the size and ratio are set in the export dialog, which again can cause misscaling.
• PME wants the export as .bmp but the UI does not help with that a lot.
• .bmp is not supported by Java in general or AoI for import, which makes the choice a kind of strange. (I'd suggest to export as transparent .png, which would be very useful in painting the texture.)
• The preview image of the mapping is wrong. The texture appears as projected.
• Processing of the preview image blocks using the mapping editor entirely during rendering, which may take a very long time. -- The process should be either running in the background or be interrupted by any user activity. (As long the preview it is not able to present the actual current mapping I'd suggest to disable it entirely)

I'm hoping that I/we can address at least some of the issues "soonish". First on my list would be the scaling bug, if I can figure out where everything happens in the first place.

This should not delay the compatibility update though.

-- Minor edit about .bmp --

[Sailsman63]

We should also look at fixing #3 when working on these.

Particular care should be taken to make sure that the saved mapping data is still compatible with old saved files - otherwise, it's a major version update/needs to import the old version.

I'm a little surprised by this - I thought it used to work, but what would have broken it?

I'll take a bit of a look, at least in terms of familiarizing myself with what the current behavior is!

[peteihis]

The #3 was a weird bit too....

otherwise, it's a major version update/needs to import the old version.

Well, PME is a plugin, I don't see a problem there if it comes to that.

I'm a little surprised by this - I thought it used to work, but what would have broken it?

As I recall, the scaling bug has been there for a long time, but we did not realize it at first and then it took even longer to figure out how to work around it. There are still things I should check about how it behaves. I think I have located, where it happens, but don't yet know, what exactly is the mechanism.

The preview and the panel split used to be better in some earlier versions. Something unintentional seemed to happen with the very last updates, that François did. The unfolded mesh should first appear on a 512x512 pixel canvas, now the window is split in half an the canvas is shown only partially (And when you start stretching the dialog / the panels, the scaling bug appears...)

Can't say why the mapping appeared projected (SW- vs. GL-rendering?) or since when.

[peteihis]

So far happened:

The view (canvas) coordinates and the mapping coordinates are not properly separated, which is the root of all evil in mapping mismatches.

The exported image was created by the same method, that is used to update the mapping canvas. The process was rather cumbersome storing the current view coordinate values into variables, replacing them by mapping coordinates, creating the image and then assigning the old values back. As if that was not complex enough, the painting method was doing scaling of it's own, which seems to be where everything finally went irreversibly wrong.

Writing a method that, is dedicated to producing the exported image was mostly just clearing unnecessary stuff off a copy of the painting method. Export is now in .png and I'll add a selection to export the mapping with transparent, white or textured background.

edit → As a strange detail in the code: Somewhere it looks like it calculates the center coordinate of the image, takes that as the origin and then adds/subtracts half of the image size to/from the coordinate apparently resulting the original point.... Maybe I don't understand it all yet.. ← edit

Another problem is the "Auto" button. It is just thinking to much... The function seems to try to fit the mapping onto the UI and to rescale the mapping to the texture in one process which may lead to a different result than the "Fit Mapping to Image Texture Size". This can damage an already fit mapping --> The Auto button in it's current form has to go. Instead I'm planning to add a View menu with a set of Fit-functions that don't interfere with the mapping.

And more: The mapping canvas is supposed to appear in 512x512 pixels and the preview image as 150x150 pixels at mapping dialog launch but the UI is defined so that pack() reserves too little space for the elements to begin with and then just splits the available area in half, ignoring the sizes of the elements. The mapping canvas is only partially shown and the preview image is a lot larger than it needs to be. I quietly suspect, that this is somehow a result of the xml-UI. Whether it is or not, I'll probably end up reconstructing it from scratch.

And yet more:

-The preview image of the mapping is wrong. The texture appears as projected.

Sometimes it does, sometimes not. I haven't tried to find out, why but no obvious reason has popped up either.

[peteihis]

And more: The mapping canvas...

To my surprise I found fixes to those, now starting in sizes 600x600 and 200x200, and the scroll bars hidden at start. The scroll bars seem a kind of unnecessary to me though. You can zoom and move the view by mouse just like the 3D-views so really they are just graphic weight. I think I'll switch them off entirely.

[Sailsman63]

Thanks for looking at these!

It sounds like you have several different issues on your plate. Please remember to commit them separately...

At the least, keep the scaling issues separate from the pure UI stuff as much as possible. Keeping the PRs small and narrowly focused makes it easier for me to review them in a reasonable timeframe.

[peteihis]

It sounds like you have several different issues on your plate.

So far I have been doing this as a trial, where I do what ever catches my eye. It is quite impossible to keep things that well separated when you don't know what is to come. I know I'll have to split it to a few PRs and I'll starting with a reformatting case that I'll base the rest on. :)

Question: François' style to write a class has been, that starting from top there are first a bunch of methods and sub classes, then come private and public fields and the class constructor and after that more methods... I'm feeling very tempted to move the fields and the constructor to the top and the sub classes last. Any objections to that?

[peteihis]

Keeping the PRs small and narrowly focused

Dug a little forward... The things can be split into a few PRs but for an optimal result the PRs should probably be chained, each one based on the previous one. Reason: Solving the export scaling bug would seem to allow simplifying the UI-drawing code in a way, that is not possible in a parallel PR, where the buggy part is calling the same code.

On the local git it easy to branch from a finished branch, but how does it behave in the upstream git? -- Of course I'd mention in the cover text, what happens before each PR.

A more conventional way would be to do them from the same master and, when they are all merged, then do a cleanup PR, but work wise that seems a bit redundant to me...

[Sailsman63]

Then let's Daisy chain. Simple stuff first, the small changes that make other changes possible. The basic bug fixes.
Once they are merged, a new PR with the next "simple" changes. (They should be simpler at that point, right?)

I can review on a shorter loop that way:

Say I can review a certain straightforward change of about 50 lines in 25-30 min (in theory).
If the change-set is tripled, and is affecting two different behaviours, plus doing some reformatting, it might require an entire afternoon (in a single block- I can't stop in the middle!) For me to tease through and understand what is being done.

RE: class organization. Probably will prefer to do this. It needs to be separate, though. Maybe wait until I've finished reviewing/merging our current PRs.

[peteihis]

RE: class organization. Probably will prefer to do this. It needs to be separate, though. Maybe wait until I've finished reviewing/merging our current PRs.

My idea would have been to do that as a part of the firs PR of the chain to the files that would be affected. It'd be much nicer to work on a file where you can find stuff in logical places. (Though I have somewhat learned to rely more on Ctrl+F to navigate in the file...). Other than rearranging classes/methods the reformatting will mostly be deleting empty lines and removing (nowadays) unnecessary line changes. Just making it nicer to handle on a larger screen. I don't see much point in complete AoI-standardization as most of the code will remain without functional editing.

Add:

The affected files in the UV-mapping update set, I have been looking at would be UVMappingEditorDialog.java and UVMappingCanvas.java. There aren't any changes to those pending in the current PRs.

[Sailsman63]

There aren't any changes to those pending in the current PRs.

Then go for it. :) If it's just rearranging lines, it's simple to review.

(nowadays) unnecessary line changes.

?? I'd have to see what you mean.

[peteihis]

Then go for it. :)

Thanks.

?? I'd have to see what you mean.

You will. :)

            mappingCB.addEventLink(ValueChangedEvent.class, this,
                    "doMappingChanged");

into

            mappingCB.addEventLink(ValueChangedEvent.class, this, "doMappingChanged");

The line width has mostly been forced under 80 characters. There might have been reasons for that in the past but now it is only damaging readability and leaving most of the editor area empty. There are still some very long method calls need to be wrapped but mostly it's like above.

[peteihis]

This can damage an already fit mapping --> The Auto button in it's current form has to go.

It seems that, after all, the problem was not in what the Auto button did but possibly somewhere in the way, that values were swapped and then resumed for image export. Another distraction was the lack of any visual reference to size as long as there was no texture on the canvas. Also when you switch between mappings, auto fit is not used, which is a kind of confusing.

Now the UVMapper has it's own undo-system, I'll need to understand that to work on fit-methods.

[Sailsman63]

The line width has mostly been forced under 80 characters. There might have been reasons for that in the past but now it is only damaging readability

I feel like we've had part of this conversation before... You and I must process text very differently. Or perhaps the the difference is in how our first languages trained us?

Anything over about 75 characters starts being hard for me to process as a unit, unless there are intelligent line breaks. Your example here would be fine in one line, but longer argument lists really should be broken up - I often use an editor window that only had space for ~85 characters.

[Sailsman63]

I just remembered something about UV mapping in Polymesh - it kind of goes it's in way, rather than extending the API core mapping functions.

I know you've already started work on fixing these bugs, b but I'm wondering what it would take to standardise?

[peteihis]

Now I'm working on some functional and usability issues, that are not so much related to the system behind it but I'm not sure, what you actually mean by standardizing it? It does use UVMapping, Texture2D, TextureParameter etc. as they are. The rest is just tools, that are way more powerful and usable than, what core AoI can offer so far.

Sometimes the system does not seem very intuitive and I'm a bit pondering, if something could be done about it. There are things in the code, that I find a bit confusing, like that there seem be one or two layers too many in the processing of the on-screen scaling, but in the big picture it all works pretty well.

If you were referring for example to the undo system of UVMapping, it after all looks pretty straight forward to me and it just might not be worth to teach AoI-undo how to handle the PMEUVMapper's content. (I have never felt very comfortable with AoI's undo....)

At least for the time being I'll put the effort on the practical issues, that the users have to deal with and leave any deeper level contemplations to an undefined point in the future. :)

BTW: Found a pretty embarrassing bug in, what I already pushed once... Got it figured out and I'll start to reconstruct the branch. (Feeling tempted to squash it all into one big commit, but there is a lot in there then.)

I feel like we've had part of this conversation before...

Good old subject, why not? :)

You and I must process text very differently.

Probably not, when text is really text. I like to pull my e-mail window to some 15 cm width, when I read something but program code hardly is text in the conventional sense of the word. It's a machine a flow chart and a wiring scheme... You (well, I....) don't read it like a book.

Most of program code tends to be pretty narrow just by nature, but then in places like UI-definition there often are sequences with a very similar line repeating several times over. In those cases dividing the lines only would damage the graphic presentation. See this :)

You most likely look at only one word or one number at a time, when working on it and it's easiest, when you immediately know where to find it.

[peteihis]

there seem be one or two layers too many in the processing of the on-screen scaling

Of course everything is about 2% as complex as in 3D but still... It is not my highest priority now but I think, I should add a layer to the data itself and simplify the processing. The system works as it is but the model space and view space have a dependency, that should not be there.

[peteihis]

Managing more that one UV-mapped textures is virtually impossible.

I had a feeling, that the logic was somehow inverted as it says to select the mapping for the current texture. That is basically what should happen, but it is not possible to select the texture before selecting a mapping, that already is assigned to another texture.... To get the assignment to happen to more than one texture-mapping pairs, you need to cheat the system by changing the assignment of the first mapping and then swapping mappings and textures in term until they eventually appear right.

And, when you get those right, it is not over.

What I said before, that some times the texture appears projected in spite of the mapping, now may happen to the new texture. That a mapping and a texture are correctly assigned as pairs and appear as they should, on the editor, does not mean that any mapping would yet have happened. It helps to OK the editor, reopen it an OK again. -- Did not test this very thoroughly yet but what ever happens is definitely what you'd expect.....

That is probably nothing big but it is in a different part of the plugin.