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

Refactor: Split node_3d_editor_plugin in multiple files #70283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

quentinguidee
Copy link
Contributor

I would like to make some improvements this week to the 3D viewport.

However, the node_3d_editor_plugin contains a lot of "unrelated" things, which led to 8780 lines, way too much to work properly.

This file containing 7 classes with an header of 950 lines, this PR split in 7 files, as advised in this issue: #33027 (comment) (3.).

Feel free to point out things to improve.

I have not edited any methods for this PR to simplify the review, except for the constants that I had to move to Node3DEditor.

@quentinguidee quentinguidee requested a review from a team as a code owner December 19, 2022 03:35
@quentinguidee quentinguidee marked this pull request as draft December 19, 2022 03:36
@quentinguidee
Copy link
Contributor Author

Does someone know why the CI static checks fail on this?

-/*************************************************************************/
+/*************************************************************************/

@Riteo
Copy link
Contributor

Riteo commented Dec 19, 2022

@quentinguidee there's some weird junk at the beginning of the deleted line:

From `hexdump -Cv':

00000000  2d ef bb bf 2f 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |-.../***********|
00000010  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000020  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000030  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000040  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2f 0a  |**************/.|
00000050  2b 2f 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |+/**************|
00000060  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000070  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000080  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2a 2a 2a 2a 2a  |****************|
00000090  2a 2a 2a 2a 2a 2a 2a 2a  2a 2a 2a 2f              |***********/|
0000009c

I have no idea why the stuff marked as 2d ef bb bf is invisible, but it definitely confuses the hell out of my text editor.

Edit: 2d is actually -, so the troublesome part is just ef bb bf.

@Riteo
Copy link
Contributor

Riteo commented Dec 19, 2022

Ok update: I used a fancy website and that's actually a single character: EF BB BF, also known as the infamous ZERO WIDTH NO-BREAK SPACE. I have no idea on how you added ZW spaces, but that's the culprit.

I should really learn to "decode" utf8 from hexdumps sometime.

@quentinguidee
Copy link
Contributor Author

quentinguidee commented Dec 19, 2022

Thanks for your help! With this I found that JetBrains uses UTF-8 with what internet call "BOM" (UTF-8 BOM). I'll try to push the same code in 5min with UTF-8 classic to see if it fixes this.

Edit: let's see...

@Riteo
Copy link
Contributor

Riteo commented Dec 19, 2022

Uh, the infamous BOM. Looking online, I found why it parsed as a ZWNBSP:

From https://en.wikipedia.org/wiki/Zero_width_no-break_space

The word joiner replaces the zero-width no-break space (ZWNBSP, U+FEFF), as a usage of the no-break space of zero width. Character U+FEFF is intended for use as a byte order mark (BOM) at the start of a file. However, if encountered elsewhere, it should, according to Unicode, be treated as a zero-width no-break space. The deliberate use of U+FEFF for this purpose is deprecated as of Unicode 3.2, with the word joiner strongly preferred.[1][2]

Damn you BOM!

Edit: yup, can confirm that it is indeed FEFF, I read the wrong column; that website also listed the plain binary UTF-8 data 😅

@quentinguidee quentinguidee force-pushed the refactor/split_node_3d_editor branch 2 times, most recently from 5da2d5f to da2c82a Compare December 19, 2022 04:28
@quentinguidee
Copy link
Contributor Author

Thanks! Static checks pass. Now let's face some fun compiling issues 😑

@quentinguidee quentinguidee marked this pull request as ready for review December 19, 2022 04:45
@quentinguidee
Copy link
Contributor Author

Ok, Seems ready for review now.

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

Successfully merging this pull request may close these issues.

3 participants